From 3f988d2c28be7ab9d5b462ecccb79f697c1dc35a Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Wed, 5 Sep 2018 12:22:51 +0900 Subject: [PATCH 01/11] Revert refactoring of Account.USER_NAME_PATTERN In changes I02ce9c6b6 ("Optimize USER_NAME_PATTERN string and its usage") and I8082b2ad3 ("Account.java: introduce compiled pattern and use where applicable"), the usage of Account.USER_NAME_PATTERN was refactored. This refactoring conflicts with separate refactoring that was done on the master branch. Instead of trying to resolve the conflicts, which ends up with the changes effectively being reverted, just revert them here. The refactoring that was done on master can then be backported here. This reverts commit 1ee03aa948553654283702220cbf36798ac1e45e. This reverts commit cf97d694febd26aeea66c6f16355a721860f80fd. Change-Id: I199d2f1531ec2b59d4263f1b72f1c967913bb9a5 --- gerrit-reviewdb/BUILD | 1 - .../gerrit/reviewdb/client/Account.java | 20 +++++++++---------- .../server/account/AccountResolver.java | 3 +-- .../gerrit/server/account/ChangeUserName.java | 6 ++++-- .../gerrit/server/account/CreateAccount.java | 6 ++---- .../account/InvalidUserNameException.java | 5 +---- .../server/args4j/AccountIdHandler.java | 4 +--- .../gerrit/server/group/AddMembers.java | 4 +--- 8 files changed, 19 insertions(+), 30 deletions(-) diff --git a/gerrit-reviewdb/BUILD b/gerrit-reviewdb/BUILD index 67801447b5..35c1535dad 100644 --- a/gerrit-reviewdb/BUILD +++ b/gerrit-reviewdb/BUILD @@ -16,7 +16,6 @@ gwt_module( visibility = ["//visibility:public"], deps = [ "//gerrit-extension-api:client", - "//lib:guava", "//lib:gwtorm-client", "//lib:gwtorm-client_src", ], diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Account.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Account.java index 3bfdd6cac2..f1c7056d0f 100644 --- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Account.java +++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Account.java @@ -16,13 +16,11 @@ package com.google.gerrit.reviewdb.client; import static com.google.gerrit.reviewdb.client.RefNames.REFS_USERS; -import com.google.common.annotations.GwtIncompatible; import com.google.gerrit.extensions.client.DiffPreferencesInfo; import com.google.gerrit.extensions.client.GeneralPreferencesInfo; import com.google.gwtorm.client.Column; import com.google.gwtorm.client.IntKey; import java.sql.Timestamp; -import java.util.regex.Pattern; /** * Information about a single user. @@ -46,14 +44,15 @@ import java.util.regex.Pattern; * */ public final class Account { - private static final String USER_NAME_COMMON_PATTERN = "a-zA-Z0-9"; - public static final String USER_NAME_PATTERN_FIRST = "[" + USER_NAME_COMMON_PATTERN + "]"; - public static final String USER_NAME_PATTERN_REST = "[" + USER_NAME_COMMON_PATTERN + "._@-]"; - public static final String USER_NAME_PATTERN_LAST = USER_NAME_PATTERN_FIRST; + public static final String USER_NAME_PATTERN_FIRST = "[a-zA-Z0-9]"; + public static final String USER_NAME_PATTERN_REST = "[a-zA-Z0-9._@-]"; + public static final String USER_NAME_PATTERN_LAST = "[a-zA-Z0-9]"; /** Regular expression that {@link #userName} must match. */ public static final String USER_NAME_PATTERN = - "^(" + "^" + + // + "(" + // USER_NAME_PATTERN_FIRST + // @@ -66,10 +65,9 @@ public final class Account { + // USER_NAME_PATTERN_FIRST + // - ")$"; - - @GwtIncompatible("Unemulated class java.util.regex.Pattern") - public static final Pattern USER_NAME_PATTERN_COMPILED = Pattern.compile(USER_NAME_PATTERN); + ")" + + // + "$"; /** Key local to Gerrit to identify a user. */ public static class Id extends IntKey> { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountResolver.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountResolver.java index 90c876798d..9803143247 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountResolver.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountResolver.java @@ -14,7 +14,6 @@ package com.google.gerrit.server.account; -import static com.google.gerrit.reviewdb.client.Account.USER_NAME_PATTERN_COMPILED; import static java.util.stream.Collectors.toSet; import com.google.gerrit.reviewdb.client.Account; @@ -106,7 +105,7 @@ public class AccountResolver { return Collections.emptySet(); } - if (USER_NAME_PATTERN_COMPILED.matcher(nameOrEmail).matches()) { + if (nameOrEmail.matches(Account.USER_NAME_PATTERN)) { AccountState who = byId.getByUsername(nameOrEmail); if (who != null) { return Collections.singleton(who.getAccount().getId()); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/ChangeUserName.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/ChangeUserName.java index ff3de30249..c9a7aabf21 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/ChangeUserName.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/ChangeUserName.java @@ -14,12 +14,12 @@ package com.google.gerrit.server.account; -import static com.google.gerrit.reviewdb.client.Account.USER_NAME_PATTERN_COMPILED; import static com.google.gerrit.server.account.ExternalId.SCHEME_USERNAME; import static java.util.stream.Collectors.toSet; import com.google.gerrit.common.Nullable; import com.google.gerrit.common.errors.NameAlreadyUsedException; +import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.ssh.SshKeyCache; @@ -31,6 +31,7 @@ import com.google.inject.assistedinject.Assisted; import java.io.IOException; import java.util.Collection; import java.util.concurrent.Callable; +import java.util.regex.Pattern; import org.eclipse.jgit.errors.ConfigInvalidException; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -38,6 +39,7 @@ import org.slf4j.LoggerFactory; /** Operation to change the username of an account. */ public class ChangeUserName implements Callable { private static final Logger log = LoggerFactory.getLogger(ChangeUserName.class); + private static final Pattern USER_NAME_PATTERN = Pattern.compile(Account.USER_NAME_PATTERN); public static final String USERNAME_CANNOT_BE_CHANGED = "Username cannot be changed."; @@ -88,7 +90,7 @@ public class ChangeUserName implements Callable { ExternalIdsUpdate externalIdsUpdate = externalIdsUpdateFactory.create(); if (newUsername != null && !newUsername.isEmpty()) { - if (!USER_NAME_PATTERN_COMPILED.matcher(newUsername).matches()) { + if (!USER_NAME_PATTERN.matcher(newUsername).matches()) { throw new InvalidUserNameException(); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/CreateAccount.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/CreateAccount.java index c166981d67..451246ba71 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/CreateAccount.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/CreateAccount.java @@ -14,8 +14,6 @@ package com.google.gerrit.server.account; -import static com.google.gerrit.reviewdb.client.Account.USER_NAME_PATTERN; -import static com.google.gerrit.reviewdb.client.Account.USER_NAME_PATTERN_COMPILED; import static com.google.gerrit.server.account.ExternalId.SCHEME_MAILTO; import com.google.gerrit.audit.AuditService; @@ -113,9 +111,9 @@ public class CreateAccount implements RestModifyView groups = parseGroups(input.groups); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/InvalidUserNameException.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/InvalidUserNameException.java index cf7a9b8df9..d60b7af85f 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/InvalidUserNameException.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/InvalidUserNameException.java @@ -16,10 +16,7 @@ package com.google.gerrit.server.account; import com.google.gerrit.reviewdb.client.Account; -/** - * Error indicating the SSH user name does not match {@link Account#USER_NAME_PATTERN_COMPILED} - * pattern. - */ +/** Error indicating the SSH user name does not match {@link Account#USER_NAME_PATTERN} pattern. */ public class InvalidUserNameException extends Exception { private static final long serialVersionUID = 1L; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/args4j/AccountIdHandler.java b/gerrit-server/src/main/java/com/google/gerrit/server/args4j/AccountIdHandler.java index 902726ab5f..75628015bb 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/args4j/AccountIdHandler.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/args4j/AccountIdHandler.java @@ -14,8 +14,6 @@ package com.google.gerrit.server.args4j; -import static com.google.gerrit.reviewdb.client.Account.USER_NAME_PATTERN_COMPILED; - import com.google.gerrit.extensions.client.AuthType; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.server.ReviewDb; @@ -92,7 +90,7 @@ public class AccountIdHandler extends OptionHandler { } private Account.Id createAccountByLdap(String user) throws CmdLineException, IOException { - if (!USER_NAME_PATTERN_COMPILED.matcher(user).matches()) { + if (!user.matches(Account.USER_NAME_PATTERN)) { throw new CmdLineException(owner, "user \"" + user + "\" not found"); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/AddMembers.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/AddMembers.java index 6a828efe59..5c1a292cde 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/group/AddMembers.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/AddMembers.java @@ -14,8 +14,6 @@ package com.google.gerrit.server.group; -import static com.google.gerrit.reviewdb.client.Account.USER_NAME_PATTERN_COMPILED; - import com.google.common.base.Strings; import com.google.common.collect.Lists; import com.google.gerrit.audit.AuditService; @@ -200,7 +198,7 @@ public class AddMembers implements RestModifyView { } private Account createAccountByLdap(String user) throws IOException { - if (!USER_NAME_PATTERN_COMPILED.matcher(user).matches()) { + if (!user.matches(Account.USER_NAME_PATTERN)) { return null; } From 1ed9d84ceeea92ce838981b46e6f4cf412379e36 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Wed, 5 Sep 2018 11:08:30 +0900 Subject: [PATCH 02/11] AccountIT: Add basic tests for creating user with {in}valid username Change-Id: I11f433a644c5d50c7ca10c1ab3f2f1223d822330 --- .../acceptance/api/accounts/AccountIT.java | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java index 61a38302d1..f97cb2aa6a 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java @@ -852,6 +852,29 @@ public class AccountIT extends AbstractDaemonTest { gApi.accounts().id(admin.username).index(); } + @Test + public void createUserWithValidUsername() throws Exception { + ImmutableList names = + ImmutableList.of( + "user@domain", "user-name", "user_name", "1234", "user1234", "1234@domain"); + for (String name : names) { + gApi.accounts().create(name); + } + } + + @Test + public void createUserWithInvalidUsername() throws Exception { + ImmutableList invalidNames = ImmutableList.of("@", "@foo", "-", "-foo", "_", "_foo"); + for (String name : invalidNames) { + try { + gApi.accounts().create(name); + fail(String.format("Expected BadRequestException for username [%s]", name)); + } catch (BadRequestException e) { + assertThat(e).hasMessageThat().contains("must contain only"); + } + } + } + private void assertSequenceNumbers(List sshKeys) { int seq = 1; for (SshKeyInfo key : sshKeys) { From 45bf80651a397533b0f37914ddd3f89a67a4ac2d Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Fri, 2 Feb 2018 15:46:45 +0100 Subject: [PATCH 03/11] Move regular expressions for user name from Account to ExternalId User names are stored as external IDs, hence the regular expressions to validate the format of a user name should be part of the ExternalId class. There is one place in the GWT UI where user names are validated. Since the GWT UI doesn't have access to the ExternalId class it needs to copy two of the regular expressions. This should be OK since the format of user names is not expected to change and the GWT UI will be gone soon. It's actually a goal to remove all dependencies from the GWT UI on the Account class so that the Account class can be moved out of the reviewdb.client package and be turned into an immutable AutoValue type. Change-Id: I6ea7139019eb0e7f0e06efaf77ec4bd89d514006 Signed-off-by: Edwin Kempin --- .../gerrit/client/account/UsernameField.java | 11 +++++-- .../gerrit/reviewdb/client/Account.java | 25 --------------- .../server/account/AccountResolver.java | 2 +- .../gerrit/server/account/ChangeUserName.java | 5 +-- .../gerrit/server/account/CreateAccount.java | 2 +- .../gerrit/server/account/ExternalId.java | 31 +++++++++++++++++++ .../account/InvalidUserNameException.java | 7 +++-- .../server/args4j/AccountIdHandler.java | 3 +- .../gerrit/server/group/AddMembers.java | 3 +- plugins/singleusergroup | 2 +- 10 files changed, 51 insertions(+), 40 deletions(-) diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/UsernameField.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/UsernameField.java index 839a3e60cd..a717a9038e 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/UsernameField.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/UsernameField.java @@ -23,7 +23,6 @@ import com.google.gerrit.client.rpc.NativeString; import com.google.gerrit.client.rpc.RestApi; import com.google.gerrit.client.ui.OnEditEnabler; import com.google.gerrit.extensions.client.AccountFieldName; -import com.google.gerrit.reviewdb.client.Account; import com.google.gwt.event.dom.client.ClickEvent; import com.google.gwt.event.dom.client.ClickHandler; import com.google.gwt.event.dom.client.KeyCodes; @@ -38,6 +37,12 @@ import com.google.gwtexpui.globalkey.client.NpTextBox; import com.google.gwtexpui.safehtml.client.SafeHtmlBuilder; class UsernameField extends Composite { + // If these regular expressions are modified the same modifications should be done to the + // corresponding regular expressions in the + // com.google.gerrit.server.account.ExternalId class. + private static final String USER_NAME_PATTERN_FIRST_REGEX = "[a-zA-Z0-9]"; + private static final String USER_NAME_PATTERN_REST_REGEX = "[a-zA-Z0-9._@-]"; + private CopyableLabel userNameLbl; private NpTextBox userNameTxt; private Button setUserName; @@ -185,9 +190,9 @@ class UsernameField extends Composite { final TextBox box = (TextBox) event.getSource(); final String re; if (box.getCursorPos() == 0) { - re = Account.USER_NAME_PATTERN_FIRST; + re = USER_NAME_PATTERN_FIRST_REGEX; } else { - re = Account.USER_NAME_PATTERN_REST; + re = USER_NAME_PATTERN_REST_REGEX; } if (!String.valueOf(code).matches("^" + re + "$")) { event.preventDefault(); diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Account.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Account.java index f1c7056d0f..d1fcbe0c45 100644 --- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Account.java +++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Account.java @@ -44,31 +44,6 @@ import java.sql.Timestamp; * */ public final class Account { - public static final String USER_NAME_PATTERN_FIRST = "[a-zA-Z0-9]"; - public static final String USER_NAME_PATTERN_REST = "[a-zA-Z0-9._@-]"; - public static final String USER_NAME_PATTERN_LAST = "[a-zA-Z0-9]"; - - /** Regular expression that {@link #userName} must match. */ - public static final String USER_NAME_PATTERN = - "^" - + // - "(" - + // - USER_NAME_PATTERN_FIRST - + // - USER_NAME_PATTERN_REST - + "*" - + // - USER_NAME_PATTERN_LAST - + // - "|" - + // - USER_NAME_PATTERN_FIRST - + // - ")" - + // - "$"; - /** Key local to Gerrit to identify a user. */ public static class Id extends IntKey> { private static final long serialVersionUID = 1L; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountResolver.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountResolver.java index 9803143247..d0533a05e7 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountResolver.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountResolver.java @@ -105,7 +105,7 @@ public class AccountResolver { return Collections.emptySet(); } - if (nameOrEmail.matches(Account.USER_NAME_PATTERN)) { + if (nameOrEmail.matches(ExternalId.USER_NAME_PATTERN_REGEX)) { AccountState who = byId.getByUsername(nameOrEmail); if (who != null) { return Collections.singleton(who.getAccount().getId()); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/ChangeUserName.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/ChangeUserName.java index c9a7aabf21..c80e43ddf7 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/ChangeUserName.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/ChangeUserName.java @@ -19,7 +19,6 @@ import static java.util.stream.Collectors.toSet; import com.google.gerrit.common.Nullable; import com.google.gerrit.common.errors.NameAlreadyUsedException; -import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.ssh.SshKeyCache; @@ -31,7 +30,6 @@ import com.google.inject.assistedinject.Assisted; import java.io.IOException; import java.util.Collection; import java.util.concurrent.Callable; -import java.util.regex.Pattern; import org.eclipse.jgit.errors.ConfigInvalidException; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -39,7 +37,6 @@ import org.slf4j.LoggerFactory; /** Operation to change the username of an account. */ public class ChangeUserName implements Callable { private static final Logger log = LoggerFactory.getLogger(ChangeUserName.class); - private static final Pattern USER_NAME_PATTERN = Pattern.compile(Account.USER_NAME_PATTERN); public static final String USERNAME_CANNOT_BE_CHANGED = "Username cannot be changed."; @@ -90,7 +87,7 @@ public class ChangeUserName implements Callable { ExternalIdsUpdate externalIdsUpdate = externalIdsUpdateFactory.create(); if (newUsername != null && !newUsername.isEmpty()) { - if (!USER_NAME_PATTERN.matcher(newUsername).matches()) { + if (!newUsername.matches(ExternalId.USER_NAME_PATTERN_REGEX)) { throw new InvalidUserNameException(); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/CreateAccount.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/CreateAccount.java index 451246ba71..ba31678158 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/CreateAccount.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/CreateAccount.java @@ -111,7 +111,7 @@ public class CreateAccount implements RestModifyView { } private Account.Id createAccountByLdap(String user) throws CmdLineException, IOException { - if (!user.matches(Account.USER_NAME_PATTERN)) { + if (!user.matches(ExternalId.USER_NAME_PATTERN_REGEX)) { throw new CmdLineException(owner, "user \"" + user + "\" not found"); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/AddMembers.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/AddMembers.java index 5c1a292cde..2558c23060 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/group/AddMembers.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/AddMembers.java @@ -37,6 +37,7 @@ import com.google.gerrit.server.account.AccountManager; import com.google.gerrit.server.account.AccountResolver; import com.google.gerrit.server.account.AccountsCollection; import com.google.gerrit.server.account.AuthRequest; +import com.google.gerrit.server.account.ExternalId; import com.google.gerrit.server.account.GroupControl; import com.google.gerrit.server.config.AuthConfig; import com.google.gerrit.server.group.AddMembers.Input; @@ -198,7 +199,7 @@ public class AddMembers implements RestModifyView { } private Account createAccountByLdap(String user) throws IOException { - if (!user.matches(Account.USER_NAME_PATTERN)) { + if (!user.matches(ExternalId.USER_NAME_PATTERN_REGEX)) { return null; } diff --git a/plugins/singleusergroup b/plugins/singleusergroup index 570b6e287a..0d7b78fc86 160000 --- a/plugins/singleusergroup +++ b/plugins/singleusergroup @@ -1 +1 @@ -Subproject commit 570b6e287a74750d37d2a94e2cf66297c004dce4 +Subproject commit 0d7b78fc86360730df807bd3c459c1a3a095ea63 From 7a5364662fa95db1152c58459a1fa33534aeddc4 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Fri, 9 Feb 2018 14:48:19 +0100 Subject: [PATCH 04/11] Use ExternalId.isValidUsername instead of ExternalId.USER_NAME_PATTERN_REGEX Since ExternalId.USER_NAME_PATTERN_REGEX is now no longer used by external callers it is made private. Change-Id: Id7a108bf39baffc52273879bbf8973dbbea0ae0c Signed-off-by: Edwin Kempin --- .../com/google/gerrit/server/account/AccountResolver.java | 2 +- .../com/google/gerrit/server/account/ChangeUserName.java | 2 +- .../java/com/google/gerrit/server/account/CreateAccount.java | 2 +- .../java/com/google/gerrit/server/account/ExternalId.java | 2 +- .../gerrit/server/account/InvalidUserNameException.java | 5 +---- .../com/google/gerrit/server/args4j/AccountIdHandler.java | 2 +- .../main/java/com/google/gerrit/server/group/AddMembers.java | 2 +- plugins/singleusergroup | 2 +- 8 files changed, 8 insertions(+), 11 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountResolver.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountResolver.java index d0533a05e7..7e1157ccfa 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountResolver.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountResolver.java @@ -105,7 +105,7 @@ public class AccountResolver { return Collections.emptySet(); } - if (nameOrEmail.matches(ExternalId.USER_NAME_PATTERN_REGEX)) { + if (ExternalId.isValidUsername(nameOrEmail)) { AccountState who = byId.getByUsername(nameOrEmail); if (who != null) { return Collections.singleton(who.getAccount().getId()); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/ChangeUserName.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/ChangeUserName.java index c80e43ddf7..b58edfb4c6 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/ChangeUserName.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/ChangeUserName.java @@ -87,7 +87,7 @@ public class ChangeUserName implements Callable { ExternalIdsUpdate externalIdsUpdate = externalIdsUpdateFactory.create(); if (newUsername != null && !newUsername.isEmpty()) { - if (!newUsername.matches(ExternalId.USER_NAME_PATTERN_REGEX)) { + if (!ExternalId.isValidUsername(newUsername)) { throw new InvalidUserNameException(); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/CreateAccount.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/CreateAccount.java index ba31678158..f384a3502d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/CreateAccount.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/CreateAccount.java @@ -111,7 +111,7 @@ public class CreateAccount implements RestModifyView { } private Account.Id createAccountByLdap(String user) throws CmdLineException, IOException { - if (!user.matches(ExternalId.USER_NAME_PATTERN_REGEX)) { + if (!ExternalId.isValidUsername(user)) { throw new CmdLineException(owner, "user \"" + user + "\" not found"); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/AddMembers.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/AddMembers.java index 2558c23060..150ac01a69 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/group/AddMembers.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/AddMembers.java @@ -199,7 +199,7 @@ public class AddMembers implements RestModifyView { } private Account createAccountByLdap(String user) throws IOException { - if (!user.matches(ExternalId.USER_NAME_PATTERN_REGEX)) { + if (!ExternalId.isValidUsername(user)) { return null; } diff --git a/plugins/singleusergroup b/plugins/singleusergroup index 0d7b78fc86..1568d7755c 160000 --- a/plugins/singleusergroup +++ b/plugins/singleusergroup @@ -1 +1 @@ -Subproject commit 0d7b78fc86360730df807bd3c459c1a3a095ea63 +Subproject commit 1568d7755c70cdb26ddc865a7181c90f24480676 From 08fca6a7d51046fb2aa1c335ebf4cd107c122654 Mon Sep 17 00:00:00 2001 From: David Ostrovsky Date: Sun, 26 Aug 2018 15:53:01 +0200 Subject: [PATCH 05/11] Bazel: Provide toolchain with activated error prone warnings On recent bazel versions it's trivial to apply package specific checks. Provide custom java toolchain with all error prone warnings activated. The list of all error prone warning was borrowed from here: [1]. For compatibility reasons with Bazel releases two java_toolchains are added for now: * Compatible with Bazel 0.16.1: error_prone_warnings_toolchain_bazel_0.16 compatible with 0.16.1 * Compatible with Bazel@HEAD: error_prone_warnings_toolchain Test Plan: $ bazel build --java_toolchain //tools:error_prone_warnings_toolchain \ //... [1] https://github.com/bazelbuild/BUILD_file_generator/blob/master/tools/bazel_defs/java.bzl Change-Id: I9a098c2d43cbfd87dbc261ce2cf9085dec3431e8 (cherry picked from commit 4b60cd7febb0a9a1e4a511dfb1855cc50c588820) --- Documentation/dev-bazel.txt | 5 ++ tools/BUILD | 148 ++++++++++++++++++++++++++++++++++++ 2 files changed, 153 insertions(+) diff --git a/Documentation/dev-bazel.txt b/Documentation/dev-bazel.txt index caa4ba8e77..129071e188 100644 --- a/Documentation/dev-bazel.txt +++ b/Documentation/dev-bazel.txt @@ -121,6 +121,11 @@ The output JAR file will be be placed in: Note that when building an individual plugin, the `core.zip` package is not regenerated. +To build with all Error Prone warnings activated, run: + +---- + bazel build --java_toolchain //tools:error_prone_warnings_toolchain //... +---- [[IDEs]] diff --git a/tools/BUILD b/tools/BUILD index 060cbd8787..379ea6cb10 100644 --- a/tools/BUILD +++ b/tools/BUILD @@ -1,6 +1,154 @@ +load("@bazel_tools//tools/jdk:default_java_toolchain.bzl", "default_java_toolchain") + py_binary( name = "merge_jars", srcs = ["merge_jars.py"], main = "merge_jars.py", visibility = ["//visibility:public"], ) + +# TODO(davido): remove this when minimum suported Bazel version >= 0.17 +# Copied from tools/jdk/default_java_toolchain.bzl to make Bazel 0.16 +# and later Bazel released to work as expected. See this issue for context: +# https://github.com/bazelbuild/bazel/issues/6009 +JDK9_JVM_OPTS = [ + # Allow JavaBuilder to access internal javac APIs. + "--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED", + "--add-opens=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED", + + # override the javac in the JDK. + "--patch-module=java.compiler=$(location @bazel_tools//third_party/java/jdk/langtools:java_compiler_jar)", + "--patch-module=jdk.compiler=$(location @bazel_tools//third_party/java/jdk/langtools:jdk_compiler_jar)", + + # quiet warnings from com.google.protobuf.UnsafeUtil, + # see: https://github.com/google/protobuf/issues/3781 + "--add-opens=java.base/java.nio=ALL-UNNAMED", +] + +# See https://github.com/bazelbuild/bazel/issues/3427 for more context +default_java_toolchain( + name = "error_prone_warnings_toolchain_bazel_0.16", + bootclasspath = ["@bazel_tools//tools/jdk:platformclasspath.jar"], + jvm_opts = JDK9_JVM_OPTS, + package_configuration = [ + ":error_prone", + ], + visibility = ["//visibility:public"], +) + +default_java_toolchain( + name = "error_prone_warnings_toolchain", + bootclasspath = ["@bazel_tools//tools/jdk:platformclasspath8.jar"], + package_configuration = [ + ":error_prone", + ], + visibility = ["//visibility:public"], +) + +# This EP warnings list is based on: +# https://github.com/bazelbuild/BUILD_file_generator/blob/master/tools/bazel_defs/java.bzl +java_package_configuration( + name = "error_prone", + javacopts = [ + "-XepDisableWarningsInGeneratedCode", + "-Xep:MissingCasesInEnumSwitch:ERROR", + "-Xep:ReferenceEquality:WARN", + "-Xep:StringEquality:WARN", + "-Xep:WildcardImport:WARN", + "-Xep:AmbiguousMethodReference:WARN", + "-Xep:BadAnnotationImplementation:WARN", + "-Xep:BadComparable:WARN", + "-Xep:BoxedPrimitiveConstructor:ERROR", + "-Xep:CannotMockFinalClass:WARN", + "-Xep:ClassCanBeStatic:WARN", + "-Xep:ClassNewInstance:WARN", + "-Xep:DefaultCharset:WARN", + "-Xep:DoubleCheckedLocking:WARN", + "-Xep:ElementsCountedInLoop:WARN", + "-Xep:EqualsHashCode:WARN", + "-Xep:EqualsIncompatibleType:WARN", + "-Xep:ExpectedExceptionChecker:ERROR", + "-Xep:Finally:WARN", + "-Xep:FloatingPointLiteralPrecision:WARN", + "-Xep:FragmentInjection:WARN", + "-Xep:FragmentNotInstantiable:WARN", + "-Xep:FunctionalInterfaceClash:WARN", + "-Xep:FutureReturnValueIgnored:WARN", + "-Xep:GetClassOnEnum:WARN", + "-Xep:ImmutableAnnotationChecker:WARN", + "-Xep:ImmutableEnumChecker:WARN", + "-Xep:IncompatibleModifiers:WARN", + "-Xep:InjectOnConstructorOfAbstractClass:WARN", + "-Xep:InputStreamSlowMultibyteRead:WARN", + "-Xep:IterableAndIterator:WARN", + "-Xep:JUnit3FloatingPointComparisonWithoutDelta:WARN", + "-Xep:JUnitAmbiguousTestClass:WARN", + "-Xep:LiteralClassName:WARN", + "-Xep:MissingFail:WARN", + "-Xep:MissingOverride:WARN", + "-Xep:MutableConstantField:WARN", + "-Xep:NarrowingCompoundAssignment:WARN", + "-Xep:NonAtomicVolatileUpdate:WARN", + "-Xep:NonOverridingEquals:WARN", + "-Xep:NullableConstructor:WARN", + "-Xep:NullablePrimitive:WARN", + "-Xep:NullableVoid:WARN", + "-Xep:OperatorPrecedence:WARN", + "-Xep:OverridesGuiceInjectableMethod:WARN", + "-Xep:PreconditionsInvalidPlaceholder:WARN", + "-Xep:ProtoFieldPreconditionsCheckNotNull:WARN", + "-Xep:ProtocolBufferOrdinal:WARN", + "-Xep:RequiredModifiers:WARN", + "-Xep:ShortCircuitBoolean:WARN", + "-Xep:SimpleDateFormatConstant:WARN", + "-Xep:StaticGuardedByInstance:WARN", + "-Xep:SynchronizeOnNonFinalField:WARN", + "-Xep:TruthConstantAsserts:WARN", + "-Xep:TypeParameterShadowing:WARN", + "-Xep:TypeParameterUnusedInFormals:WARN", + "-Xep:URLEqualsHashCode:WARN", + "-Xep:UnsynchronizedOverridesSynchronized:WARN", + "-Xep:WaitNotInLoop:WARN", + ], + packages = ["error_prone_packages"], +) + +package_group( + name = "error_prone_packages", + packages = [ + "//gerrit-acceptance-framework/...", + "//gerrit-acceptance-tests/...", + "//gerrit-cache-h2/...", + "//gerrit-cache-mem/...", + "//gerrit-common/...", + "//gerrit-elasticsearch/...", + "//gerrit-extension-api/...", + "//gerrit-gpg/...", + "//gerrit-httpd/...", + "//gerrit-launcher/...", + "//gerrit-lucene/...", + "//gerrit-main/...", + "//gerrit-oauth/...", + "//gerrit-openid/...", + "//gerrit-patch-commonsnet/...", + "//gerrit-patch-jgit/...", + "//gerrit-pgm/...", + "//gerrit-plugin-api/...", + "//gerrit-plugin-gwtui/...", + "//gerrit-prettify/...", + "//gerrit-reviewdb/...", + "//gerrit-server/...", + "//gerrit-sshd/...", + "//gerrit-test-util/...", + "//gerrit-util-cli/...", + "//gerrit-util-http/...", + "//gerrit-util-ssl/...", + "//gerrit-war/...", + ], +) From dae9b02efee370bec1432a7910179e98f0737711 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Wed, 5 Sep 2018 15:42:44 +0900 Subject: [PATCH 06/11] CreateAccount: Simplify error message when username is invalid The exception is hardcoded to mention the valid characters of the username, but doesn't actually match the characters that are allowed since it doesn't include the '@' character. To avoid the possibility of this error message becoming even more out of synch if further characters are allowed, simplify it to only say that the username is invalid. Change-Id: I036f1690442cd12f44377f44ae728f6ecb5f1346 --- .../com/google/gerrit/acceptance/api/accounts/AccountIT.java | 2 +- .../java/com/google/gerrit/server/account/CreateAccount.java | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java index f97cb2aa6a..e5ddbbbd87 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java @@ -870,7 +870,7 @@ public class AccountIT extends AbstractDaemonTest { gApi.accounts().create(name); fail(String.format("Expected BadRequestException for username [%s]", name)); } catch (BadRequestException e) { - assertThat(e).hasMessageThat().contains("must contain only"); + assertThat(e).hasMessageThat().isEqualTo(String.format("Invalid username '%s'", name)); } } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/CreateAccount.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/CreateAccount.java index f384a3502d..e7d699466a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/CreateAccount.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/CreateAccount.java @@ -112,8 +112,7 @@ public class CreateAccount implements RestModifyView groups = parseGroups(input.groups); From 81f2b5c6f6ada7a07755c2e5f7d1f7d0aaec9f5c Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Thu, 23 Aug 2018 14:44:14 +0900 Subject: [PATCH 07/11] Allow to inherit receive.maxObjectSizeLimit from parent project In the current implementation it is possible to set the limit per project in the project.config on refs/meta/config, and at global level in $site/etc/gerrit.config. The project setting may override the global setting if it is lower. Changing the global setting requires a server restart. A limitation of this implementation is that we cannot set the limit at a project level and have it inherited to its child projects; it is necessary to explicitly set the limit on each child project. This limitation causes a lot of extra work in the case where for example we have a project hierarchy like: |- All-Projects | -- Namespace-A | | | |-- Project-A | |-- Project-B . . .. . . .. | |-- Project-X | | -- Namespace-B Where the Namespace-X projects are assumed to be "parent only" projects, if we want to set a limit for all the projects under a namespace hierarchy, we need to set it explicitly on all those projects individually rather than only on the "Namespace-X". With this change the limit is inherited from the parent project. The global limit is still respected, and the project still can't set a higher value than the global, either explicitly per project or via inheritance. Similarly, if no global limit is specified, a child project still may not set a limit higher than its parent. The inheritedValue is removed from the config info and replaced by a summary string describing how the effective value was inherited or overridden from the parent project or the global config. This string is used as the tooltip on the effective value in the UI. As a side effect of this change, it is now possible to effectively change the global limit without having to restart the server, by setting it on the All-Projects project. Note that this only works if the new limit is lower than what is already configured in the actual global limit in gerrit.config. Bug: Issue 9528 Change-Id: I5f8b333e905ed0a147526ae33ff2bab2cbe222ef --- Documentation/rest-api-projects.txt | 8 +- .../acceptance/api/project/ProjectIT.java | 105 ++++++++++++++++-- .../extensions/api/projects/ConfigInfo.java | 11 +- .../gerrit/client/admin/AdminMessages.java | 2 - .../client/admin/AdminMessages.properties | 1 - .../client/admin/ProjectInfoScreen.java | 5 +- .../gerrit/client/projects/ConfigInfo.java | 4 +- .../gerrit/server/git/ReceiveCommits.java | 3 +- .../gerrit/server/project/ConfigInfoImpl.java | 13 +-- .../gerrit/server/project/GetConfig.java | 5 - .../gerrit/server/project/ProjectState.java | 55 ++++++++- .../gerrit/server/project/PutConfig.java | 5 - 12 files changed, 167 insertions(+), 50 deletions(-) diff --git a/Documentation/rest-api-projects.txt b/Documentation/rest-api-projects.txt index eee31519f1..8e151bc45d 100644 --- a/Documentation/rest-api-projects.txt +++ b/Documentation/rest-api-projects.txt @@ -2812,10 +2812,10 @@ The max object size limit that is configured on the project as a formatted string. + Not set if there is no limit for the object size configured on project level. -|`inherited_value` |optional| -The max object size limit that is inherited from the global config as a -formatted string. + -Not set if there is no global limit for the object size. +|`summary` |optional| +A string describing whether the value was inherited or overridden from +the parent project or global config. + +Not set if not inherited or overridden. |=============================== [[project-access-input]] diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/project/ProjectIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/project/ProjectIT.java index 7c62302192..5f97a44dcd 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/project/ProjectIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/project/ProjectIT.java @@ -16,6 +16,10 @@ package com.google.gerrit.acceptance.api.project; import static com.google.common.truth.Truth.assertThat; import static com.google.gerrit.server.group.SystemGroupBackend.ANONYMOUS_USERS; +import static com.google.gerrit.server.project.ProjectState.INHERITED_FROM_GLOBAL; +import static com.google.gerrit.server.project.ProjectState.INHERITED_FROM_PARENT; +import static com.google.gerrit.server.project.ProjectState.OVERRIDDEN_BY_GLOBAL; +import static com.google.gerrit.server.project.ProjectState.OVERRIDDEN_BY_PARENT; import com.google.gerrit.acceptance.AbstractDaemonTest; import com.google.gerrit.acceptance.GerritConfig; @@ -144,7 +148,7 @@ public class ProjectIT extends AbstractDaemonTest { ConfigInfo info = getConfig(); assertThat(info.maxObjectSizeLimit.value).isNull(); assertThat(info.maxObjectSizeLimit.configuredValue).isNull(); - assertThat(info.maxObjectSizeLimit.inheritedValue).isNull(); + assertThat(info.maxObjectSizeLimit.summary).isNull(); } @Test @@ -153,37 +157,91 @@ public class ProjectIT extends AbstractDaemonTest { ConfigInfo info = setMaxObjectSize("100k"); assertThat(info.maxObjectSizeLimit.value).isEqualTo("102400"); assertThat(info.maxObjectSizeLimit.configuredValue).isEqualTo("100k"); - assertThat(info.maxObjectSizeLimit.inheritedValue).isNull(); + assertThat(info.maxObjectSizeLimit.summary).isNull(); // Clear the value info = setMaxObjectSize("0"); assertThat(info.maxObjectSizeLimit.value).isNull(); assertThat(info.maxObjectSizeLimit.configuredValue).isNull(); - assertThat(info.maxObjectSizeLimit.inheritedValue).isNull(); + assertThat(info.maxObjectSizeLimit.summary).isNull(); } @Test - public void maxObjectSizeIsNotInheritedFromParentProject() throws Exception { + public void maxObjectSizeIsInheritedFromParentProject() throws Exception { Project.NameKey child = createProject(name("child"), project); ConfigInfo info = setMaxObjectSize("100k"); assertThat(info.maxObjectSizeLimit.value).isEqualTo("102400"); assertThat(info.maxObjectSizeLimit.configuredValue).isEqualTo("100k"); - assertThat(info.maxObjectSizeLimit.inheritedValue).isNull(); + assertThat(info.maxObjectSizeLimit.summary).isNull(); info = getConfig(child); + assertThat(info.maxObjectSizeLimit.value).isEqualTo("102400"); + assertThat(info.maxObjectSizeLimit.configuredValue).isNull(); + assertThat(info.maxObjectSizeLimit.summary) + .isEqualTo(String.format(INHERITED_FROM_PARENT, project)); + } + + @Test + public void maxObjectSizeOverridesParentProjectWhenNotSetOnParent() throws Exception { + Project.NameKey child = createProject(name("child"), project); + + ConfigInfo info = setMaxObjectSize("0"); assertThat(info.maxObjectSizeLimit.value).isNull(); assertThat(info.maxObjectSizeLimit.configuredValue).isNull(); - assertThat(info.maxObjectSizeLimit.inheritedValue).isNull(); + assertThat(info.maxObjectSizeLimit.summary).isNull(); + + info = setMaxObjectSize(child, "100k"); + assertThat(info.maxObjectSizeLimit.value).isEqualTo("102400"); + assertThat(info.maxObjectSizeLimit.configuredValue).isEqualTo("100k"); + assertThat(info.maxObjectSizeLimit.summary).isNull(); + } + + @Test + public void maxObjectSizeOverridesParentProjectWhenLower() throws Exception { + Project.NameKey child = createProject(name("child"), project); + + ConfigInfo info = setMaxObjectSize("200k"); + assertThat(info.maxObjectSizeLimit.value).isEqualTo("204800"); + assertThat(info.maxObjectSizeLimit.configuredValue).isEqualTo("200k"); + assertThat(info.maxObjectSizeLimit.summary).isNull(); + + info = setMaxObjectSize(child, "100k"); + assertThat(info.maxObjectSizeLimit.value).isEqualTo("102400"); + assertThat(info.maxObjectSizeLimit.configuredValue).isEqualTo("100k"); + assertThat(info.maxObjectSizeLimit.summary).isNull(); + } + + @Test + public void maxObjectSizeDoesNotOverrideParentProjectWhenHigher() throws Exception { + Project.NameKey child = createProject(name("child"), project); + + ConfigInfo info = setMaxObjectSize("100k"); + assertThat(info.maxObjectSizeLimit.value).isEqualTo("102400"); + assertThat(info.maxObjectSizeLimit.configuredValue).isEqualTo("100k"); + assertThat(info.maxObjectSizeLimit.summary).isNull(); + + info = setMaxObjectSize(child, "200k"); + assertThat(info.maxObjectSizeLimit.value).isEqualTo("102400"); + assertThat(info.maxObjectSizeLimit.configuredValue).isEqualTo("200k"); + assertThat(info.maxObjectSizeLimit.summary) + .isEqualTo(String.format(OVERRIDDEN_BY_PARENT, project)); } @Test @GerritConfig(name = "receive.maxObjectSizeLimit", value = "200k") public void maxObjectSizeIsInheritedFromGlobalConfig() throws Exception { + Project.NameKey child = createProject(name("child"), project); + ConfigInfo info = getConfig(); assertThat(info.maxObjectSizeLimit.value).isEqualTo("204800"); assertThat(info.maxObjectSizeLimit.configuredValue).isNull(); - assertThat(info.maxObjectSizeLimit.inheritedValue).isEqualTo("200k"); + assertThat(info.maxObjectSizeLimit.summary).isEqualTo(INHERITED_FROM_GLOBAL); + + info = getConfig(child); + assertThat(info.maxObjectSizeLimit.value).isEqualTo("204800"); + assertThat(info.maxObjectSizeLimit.configuredValue).isNull(); + assertThat(info.maxObjectSizeLimit.summary).isEqualTo(INHERITED_FROM_GLOBAL); } @Test @@ -192,16 +250,39 @@ public class ProjectIT extends AbstractDaemonTest { ConfigInfo info = setMaxObjectSize("100k"); assertThat(info.maxObjectSizeLimit.value).isEqualTo("102400"); assertThat(info.maxObjectSizeLimit.configuredValue).isEqualTo("100k"); - assertThat(info.maxObjectSizeLimit.inheritedValue).isEqualTo("200k"); + assertThat(info.maxObjectSizeLimit.summary).isNull(); + } + + @Test + @GerritConfig(name = "receive.maxObjectSizeLimit", value = "300k") + public void inheritedMaxObjectSizeOverridesGlobalConfigWhenLower() throws Exception { + Project.NameKey child = createProject(name("child"), project); + + ConfigInfo info = setMaxObjectSize("200k"); + assertThat(info.maxObjectSizeLimit.value).isEqualTo("204800"); + assertThat(info.maxObjectSizeLimit.configuredValue).isEqualTo("200k"); + assertThat(info.maxObjectSizeLimit.summary).isNull(); + + info = setMaxObjectSize(child, "100k"); + assertThat(info.maxObjectSizeLimit.value).isEqualTo("102400"); + assertThat(info.maxObjectSizeLimit.configuredValue).isEqualTo("100k"); + assertThat(info.maxObjectSizeLimit.summary).isNull(); } @Test @GerritConfig(name = "receive.maxObjectSizeLimit", value = "200k") public void maxObjectSizeDoesNotOverrideGlobalConfigWhenHigher() throws Exception { + Project.NameKey child = createProject(name("child"), project); + ConfigInfo info = setMaxObjectSize("300k"); assertThat(info.maxObjectSizeLimit.value).isEqualTo("204800"); assertThat(info.maxObjectSizeLimit.configuredValue).isEqualTo("300k"); - assertThat(info.maxObjectSizeLimit.inheritedValue).isEqualTo("200k"); + assertThat(info.maxObjectSizeLimit.summary).isEqualTo(OVERRIDDEN_BY_GLOBAL); + + info = getConfig(child); + assertThat(info.maxObjectSizeLimit.value).isEqualTo("204800"); + assertThat(info.maxObjectSizeLimit.configuredValue).isNull(); + assertThat(info.maxObjectSizeLimit.summary).isEqualTo(OVERRIDDEN_BY_GLOBAL); } @Test @@ -220,9 +301,13 @@ public class ProjectIT extends AbstractDaemonTest { } private ConfigInfo setMaxObjectSize(String value) throws Exception { + return setMaxObjectSize(project, value); + } + + private ConfigInfo setMaxObjectSize(Project.NameKey name, String value) throws Exception { ConfigInput input = new ConfigInput(); input.maxObjectSizeLimit = value; - return setConfig(input); + return setConfig(name, input); } private ConfigInfo getConfig(Project.NameKey name) throws Exception { diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/projects/ConfigInfo.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/projects/ConfigInfo.java index 09ad3281a7..36c86ed433 100644 --- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/projects/ConfigInfo.java +++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/projects/ConfigInfo.java @@ -48,14 +48,17 @@ public class ConfigInfo { } public static class MaxObjectSizeLimitInfo { - /* The effective value. Null if not set. */ + /** The effective value in bytes. Null if not set. */ @Nullable public String value; - /* The value configured on the project. Null if not set. */ + /** The value configured explicitly on the project as a formatted string. Null if not set. */ @Nullable public String configuredValue; - /* The value configured globally. Null if not set. */ - @Nullable public String inheritedValue; + /** + * Whether the value was inherited or overridden from the project's parent hierarchy or global + * config. Null if not inherited or overridden. + */ + @Nullable public String summary; } public static class ConfigParameterInfo { diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/AdminMessages.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/AdminMessages.java index fe27e9c7cf..7b18a392ed 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/AdminMessages.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/AdminMessages.java @@ -36,8 +36,6 @@ public interface AdminMessages extends Messages { String effectiveMaxObjectSizeLimit(String effectiveMaxObjectSizeLimit); - String globalMaxObjectSizeLimit(String globalMaxObjectSizeLimit); - String noMaxObjectSizeLimit(); String pluginProjectOptionsTitle(String pluginName); diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/AdminMessages.properties b/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/AdminMessages.properties index f746365f18..c9aa987964 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/AdminMessages.properties +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/AdminMessages.properties @@ -6,7 +6,6 @@ deletedGroup = Deleted Group {0} deletedReference = Reference {0} was deleted deletedSection = Section {0} was deleted effectiveMaxObjectSizeLimit = effective: {0} bytes -globalMaxObjectSizeLimit = The global max object size limit is set to {0}. The limit cannot be increased on project level. noMaxObjectSizeLimit = No max object size limit is set. pluginProjectOptionsTitle = {0} Plugin Options pluginProjectOptionsTitle = {0} Plugin diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/ProjectInfoScreen.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/ProjectInfoScreen.java index 0e185eb130..2e4054ee63 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/ProjectInfoScreen.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/ProjectInfoScreen.java @@ -404,9 +404,8 @@ public class ProjectInfoScreen extends ProjectScreen { if (result.maxObjectSizeLimit().value() != null) { effectiveMaxObjectSizeLimit.setText( AdminMessages.I.effectiveMaxObjectSizeLimit(result.maxObjectSizeLimit().value())); - if (result.maxObjectSizeLimit().inheritedValue() != null) { - effectiveMaxObjectSizeLimit.setTitle( - AdminMessages.I.globalMaxObjectSizeLimit(result.maxObjectSizeLimit().inheritedValue())); + if (result.maxObjectSizeLimit().summary() != null) { + effectiveMaxObjectSizeLimit.setTitle(result.maxObjectSizeLimit().summary()); } } else { effectiveMaxObjectSizeLimit.setText(AdminMessages.I.noMaxObjectSizeLimit()); diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/projects/ConfigInfo.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/projects/ConfigInfo.java index 738319d410..c96d331494 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/projects/ConfigInfo.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/projects/ConfigInfo.java @@ -158,10 +158,10 @@ public class ConfigInfo extends JavaScriptObject { public static class MaxObjectSizeLimitInfo extends JavaScriptObject { public final native String value() /*-{ return this.value; }-*/; - public final native String inheritedValue() /*-{ return this.inherited_value; }-*/; - public final native String configuredValue() /*-{ return this.configured_value }-*/; + public final native String summary() /*-{ return this.summary; }-*/; + protected MaxObjectSizeLimitInfo() {} } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java index cd974647dd..4bd51cfe5e 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java @@ -446,7 +446,8 @@ public class ReceiveCommits { rp.setAllowNonFastForwards(true); rp.setRefLogIdent(user.newRefLogIdent()); rp.setTimeout(transferConfig.getTimeout()); - rp.setMaxObjectSizeLimit(projectControl.getProjectState().getEffectiveMaxObjectSizeLimit()); + rp.setMaxObjectSizeLimit( + projectControl.getProjectState().getEffectiveMaxObjectSizeLimit().value); rp.setCheckReceivedObjects(ps.getConfig().getCheckReceivedObjects()); rp.setRefFilter( new RefFilter() { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ConfigInfoImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ConfigInfoImpl.java index 24760b7b10..62b8c9df96 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ConfigInfoImpl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ConfigInfoImpl.java @@ -30,7 +30,7 @@ import com.google.gerrit.server.config.PluginConfig; import com.google.gerrit.server.config.PluginConfigFactory; import com.google.gerrit.server.config.ProjectConfigEntry; import com.google.gerrit.server.extensions.webui.UiActions; -import com.google.gerrit.server.git.TransferConfig; +import com.google.gerrit.server.project.ProjectState.EffectiveMaxObjectSizeLimit; import com.google.inject.util.Providers; import java.util.Arrays; import java.util.LinkedHashMap; @@ -41,7 +41,6 @@ public class ConfigInfoImpl extends ConfigInfo { public ConfigInfoImpl( boolean serverEnableSignedPush, ProjectControl control, - TransferConfig transferConfig, DynamicMap pluginConfigEntries, PluginConfigFactory cfgFactory, AllProjectsName allProjects, @@ -98,7 +97,7 @@ public class ConfigInfoImpl extends ConfigInfo { this.requireSignedPush = requireSignedPush; } - this.maxObjectSizeLimit = getMaxObjectSizeLimit(projectState, transferConfig, p); + this.maxObjectSizeLimit = getMaxObjectSizeLimit(projectState, p); this.submitType = p.getSubmitType(); this.state = @@ -122,13 +121,13 @@ public class ConfigInfoImpl extends ConfigInfo { this.theme = projectState.getTheme(); } - private MaxObjectSizeLimitInfo getMaxObjectSizeLimit( - ProjectState projectState, TransferConfig transferConfig, Project p) { + private MaxObjectSizeLimitInfo getMaxObjectSizeLimit(ProjectState projectState, Project p) { MaxObjectSizeLimitInfo info = new MaxObjectSizeLimitInfo(); - long value = projectState.getEffectiveMaxObjectSizeLimit(); + EffectiveMaxObjectSizeLimit limit = projectState.getEffectiveMaxObjectSizeLimit(); + long value = limit.value; info.value = value == 0 ? null : String.valueOf(value); info.configuredValue = p.getMaxObjectSizeLimit(); - info.inheritedValue = transferConfig.getFormattedMaxObjectSizeLimit(); + info.summary = limit.summary; return info; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/GetConfig.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/GetConfig.java index 08df3863f9..1bf001b3f8 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/GetConfig.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/GetConfig.java @@ -22,14 +22,12 @@ import com.google.gerrit.server.EnableSignedPush; import com.google.gerrit.server.config.AllProjectsName; import com.google.gerrit.server.config.PluginConfigFactory; import com.google.gerrit.server.config.ProjectConfigEntry; -import com.google.gerrit.server.git.TransferConfig; import com.google.inject.Inject; import com.google.inject.Singleton; @Singleton public class GetConfig implements RestReadView { private final boolean serverEnableSignedPush; - private final TransferConfig transferConfig; private final DynamicMap pluginConfigEntries; private final PluginConfigFactory cfgFactory; private final AllProjectsName allProjects; @@ -38,13 +36,11 @@ public class GetConfig implements RestReadView { @Inject public GetConfig( @EnableSignedPush boolean serverEnableSignedPush, - TransferConfig transferConfig, DynamicMap pluginConfigEntries, PluginConfigFactory cfgFactory, AllProjectsName allProjects, DynamicMap> views) { this.serverEnableSignedPush = serverEnableSignedPush; - this.transferConfig = transferConfig; this.pluginConfigEntries = pluginConfigEntries; this.allProjects = allProjects; this.cfgFactory = cfgFactory; @@ -56,7 +52,6 @@ public class GetConfig implements RestReadView { return new ConfigInfoImpl( serverEnableSignedPush, resource.getControl(), - transferConfig, pluginConfigEntries, cfgFactory, allProjects, diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectState.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectState.java index 9e9526dfdc..2e3677dea6 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectState.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectState.java @@ -17,6 +17,7 @@ package com.google.gerrit.server.project; import static com.google.gerrit.common.data.PermissionRule.Action.ALLOW; import static java.nio.charset.StandardCharsets.UTF_8; +import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.FluentIterable; import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; @@ -245,13 +246,55 @@ public class ProjectState { return cfg; } - public long getEffectiveMaxObjectSizeLimit() { - long local = config.getMaxObjectSizeLimit(); - if (globalMaxObjectSizeLimit > 0 && local > 0) { - return Math.min(globalMaxObjectSizeLimit, local); + public static class EffectiveMaxObjectSizeLimit { + public long value; + public String summary; + } + + private static final String MAY_NOT_SET = "This project may not set a higher limit."; + + @VisibleForTesting + public static final String INHERITED_FROM_PARENT = "Inherited from parent project '%s'."; + + @VisibleForTesting + public static final String OVERRIDDEN_BY_PARENT = + "Overridden by parent project '%s'. " + MAY_NOT_SET; + + @VisibleForTesting + public static final String INHERITED_FROM_GLOBAL = "Inherited from the global config."; + + @VisibleForTesting + public static final String OVERRIDDEN_BY_GLOBAL = + "Overridden by the global config. " + MAY_NOT_SET; + + public EffectiveMaxObjectSizeLimit getEffectiveMaxObjectSizeLimit() { + EffectiveMaxObjectSizeLimit result = new EffectiveMaxObjectSizeLimit(); + + result.value = config.getMaxObjectSizeLimit(); + for (ProjectState parent : parents()) { + long parentValue = parent.config.getMaxObjectSizeLimit(); + if (parentValue > 0 && result.value > 0) { + if (parentValue < result.value) { + result.value = parentValue; + result.summary = String.format(OVERRIDDEN_BY_PARENT, parent.config.getName()); + } + } else if (parentValue > 0) { + result.value = parentValue; + result.summary = String.format(INHERITED_FROM_PARENT, parent.config.getName()); + } } - // zero means "no limit", in this case the max is more limiting - return Math.max(globalMaxObjectSizeLimit, local); + + if (globalMaxObjectSizeLimit > 0 && result.value > 0) { + if (globalMaxObjectSizeLimit < result.value) { + result.value = globalMaxObjectSizeLimit; + result.summary = OVERRIDDEN_BY_GLOBAL; + } + } else if (globalMaxObjectSizeLimit > result.value) { + // zero means "no limit", in this case the max is more limiting + result.value = globalMaxObjectSizeLimit; + result.summary = INHERITED_FROM_GLOBAL; + } + return result; } /** Get the sections that pertain only to this project. */ diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/PutConfig.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/PutConfig.java index 4ec8a7703c..85dcfbc931 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/PutConfig.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/PutConfig.java @@ -36,7 +36,6 @@ import com.google.gerrit.server.config.PluginConfigFactory; import com.google.gerrit.server.config.ProjectConfigEntry; import com.google.gerrit.server.git.MetaDataUpdate; import com.google.gerrit.server.git.ProjectConfig; -import com.google.gerrit.server.git.TransferConfig; import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.Singleton; @@ -58,7 +57,6 @@ public class PutConfig implements RestModifyView { private final Provider metaDataUpdateFactory; private final ProjectCache projectCache; private final ProjectState.Factory projectStateFactory; - private final TransferConfig transferConfig; private final DynamicMap pluginConfigEntries; private final PluginConfigFactory cfgFactory; private final AllProjectsName allProjects; @@ -71,7 +69,6 @@ public class PutConfig implements RestModifyView { Provider metaDataUpdateFactory, ProjectCache projectCache, ProjectState.Factory projectStateFactory, - TransferConfig transferConfig, DynamicMap pluginConfigEntries, PluginConfigFactory cfgFactory, AllProjectsName allProjects, @@ -81,7 +78,6 @@ public class PutConfig implements RestModifyView { this.metaDataUpdateFactory = metaDataUpdateFactory; this.projectCache = projectCache; this.projectStateFactory = projectStateFactory; - this.transferConfig = transferConfig; this.pluginConfigEntries = pluginConfigEntries; this.cfgFactory = cfgFactory; this.allProjects = allProjects; @@ -176,7 +172,6 @@ public class PutConfig implements RestModifyView { return new ConfigInfoImpl( serverEnableSignedPush, state.controlFor(user.get()), - transferConfig, pluginConfigEntries, cfgFactory, allProjects, From 3f9c740c4a18da4380b91bc9120fcccf1b6e35c0 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Wed, 5 Sep 2018 18:43:03 +0900 Subject: [PATCH 08/11] Make inheritance of receive.maxObjectSizeLimit optional Change I5f8b333e9 made the receive.maxObjectSizeLimit setting inherited from the parent project. However this is a change in behavior that users might not expect, and could have unexpected results. Rather than making this new behavior the default, make it so that it must be explicitly enabled in the site config by setting a new value, receive.inheritProjectMaxObjectSizeLimit, to true. Default the value to false when not explicitly set, to maintain backward compatibility with the behavior before change I5f8b333e9. Bug: Issue 9528 Change-Id: I63a990a61f0428148a5d26f9cf539d7d419b21ae --- Documentation/config-gerrit.txt | 8 +++++++ Documentation/config-project-config.txt | 6 ++++-- .../acceptance/api/project/ProjectIT.java | 18 ++++++++++++++++ .../gerrit/server/git/TransferConfig.java | 7 +++++++ .../gerrit/server/project/ProjectState.java | 21 ++++++++++++------- 5 files changed, 50 insertions(+), 10 deletions(-) diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt index e4dfde491a..eae7b5c4a6 100644 --- a/Documentation/config-gerrit.txt +++ b/Documentation/config-gerrit.txt @@ -3494,6 +3494,14 @@ Default is zero. + Common unit suffixes of 'k', 'm', or 'g' are supported. +[[receive.inheritProjectMaxObjectSizeLimit]]receive.inheritProjectMaxObjectSizeLimit:: ++ +Controls whether the project-level link:config-project-config.html[`receive.maxObjectSizeLimit`] +value is inherited from the parent project. When `true`, the value is +inherited, otherwise it is not inherited. ++ +Default is false, the value is not inherited. + [[receive.maxTrustDepth]]receive.maxTrustDepth:: + If signed push validation is link:#receive.enableSignedPush[enabled], diff --git a/Documentation/config-project-config.txt b/Documentation/config-project-config.txt index faee4e616a..b9f68ed95d 100644 --- a/Documentation/config-project-config.txt +++ b/Documentation/config-project-config.txt @@ -144,8 +144,10 @@ The project specific setting in `project.config` may not set a value higher than the global limit (if configured). In other words, it is only honored when it further reduces the global limit. + -The setting is not inherited from the parent project; it must be explicitly -set per project. +When link:config-gerrit.html#receive.inheritProjectMaxObjectSizeLimit[ +`receive.inheritProjectmaxObjectSizeLimit`] is enabled in the global config, +the value is inherited from the parent project. Otherwise, it is not inherited +and must be explicitly set per project. + Default is zero. + diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/project/ProjectIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/project/ProjectIT.java index 5f97a44dcd..56bf5546d2 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/project/ProjectIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/project/ProjectIT.java @@ -167,6 +167,7 @@ public class ProjectIT extends AbstractDaemonTest { } @Test + @GerritConfig(name = "receive.inheritProjectMaxObjectSizeLimit", value = "true") public void maxObjectSizeIsInheritedFromParentProject() throws Exception { Project.NameKey child = createProject(name("child"), project); @@ -182,6 +183,21 @@ public class ProjectIT extends AbstractDaemonTest { .isEqualTo(String.format(INHERITED_FROM_PARENT, project)); } + @Test + public void maxObjectSizeIsNotInheritedFromParentProject() throws Exception { + Project.NameKey child = createProject(name("child"), project); + + ConfigInfo info = setMaxObjectSize("100k"); + assertThat(info.maxObjectSizeLimit.value).isEqualTo("102400"); + assertThat(info.maxObjectSizeLimit.configuredValue).isEqualTo("100k"); + assertThat(info.maxObjectSizeLimit.summary).isNull(); + + info = getConfig(child); + assertThat(info.maxObjectSizeLimit.value).isNull(); + assertThat(info.maxObjectSizeLimit.configuredValue).isNull(); + assertThat(info.maxObjectSizeLimit.summary).isNull(); + } + @Test public void maxObjectSizeOverridesParentProjectWhenNotSetOnParent() throws Exception { Project.NameKey child = createProject(name("child"), project); @@ -213,6 +229,7 @@ public class ProjectIT extends AbstractDaemonTest { } @Test + @GerritConfig(name = "receive.inheritProjectMaxObjectSizeLimit", value = "true") public void maxObjectSizeDoesNotOverrideParentProjectWhenHigher() throws Exception { Project.NameKey child = createProject(name("child"), project); @@ -271,6 +288,7 @@ public class ProjectIT extends AbstractDaemonTest { @Test @GerritConfig(name = "receive.maxObjectSizeLimit", value = "200k") + @GerritConfig(name = "receive.inheritProjectMaxObjectSizeLimit", value = "true") public void maxObjectSizeDoesNotOverrideGlobalConfigWhenHigher() throws Exception { Project.NameKey child = createProject(name("child"), project); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/TransferConfig.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/TransferConfig.java index 28432f56a9..773f612102 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/TransferConfig.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/TransferConfig.java @@ -28,6 +28,7 @@ public class TransferConfig { private final PackConfig packConfig; private final long maxObjectSizeLimit; private final String maxObjectSizeLimitFormatted; + private final boolean inheritProjectMaxObjectSizeLimit; @Inject TransferConfig(@GerritServerConfig final Config cfg) { @@ -42,6 +43,8 @@ public class TransferConfig { TimeUnit.SECONDS); maxObjectSizeLimit = cfg.getLong("receive", "maxObjectSizeLimit", 0); maxObjectSizeLimitFormatted = cfg.getString("receive", null, "maxObjectSizeLimit"); + inheritProjectMaxObjectSizeLimit = + cfg.getBoolean("receive", "inheritProjectMaxObjectSizeLimit", false); packConfig = new PackConfig(); packConfig.setDeltaCompress(false); @@ -65,4 +68,8 @@ public class TransferConfig { public String getFormattedMaxObjectSizeLimit() { return maxObjectSizeLimitFormatted; } + + public boolean getInheritProjectMaxObjectSizeLimit() { + return inheritProjectMaxObjectSizeLimit; + } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectState.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectState.java index 2e3677dea6..0ca4ddf64d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectState.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectState.java @@ -96,6 +96,7 @@ public class ProjectState { private final Map configs; private final Set localOwners; private final long globalMaxObjectSizeLimit; + private final boolean inheritProjectMaxObjectSizeLimit; /** Prolog rule state. */ private volatile PrologMachineCopy rulesMachine; @@ -143,6 +144,7 @@ public class ProjectState { ? capabilityFactory.create(config.getAccessSection(AccessSection.GLOBAL_CAPABILITIES)) : null; this.globalMaxObjectSizeLimit = transferConfig.getMaxObjectSizeLimit(); + this.inheritProjectMaxObjectSizeLimit = transferConfig.getInheritProjectMaxObjectSizeLimit(); if (isAllProjects && !Permission.canBeOnAllProjects(AccessSection.ALL, Permission.OWNER)) { localOwners = Collections.emptySet(); @@ -271,16 +273,19 @@ public class ProjectState { EffectiveMaxObjectSizeLimit result = new EffectiveMaxObjectSizeLimit(); result.value = config.getMaxObjectSizeLimit(); - for (ProjectState parent : parents()) { - long parentValue = parent.config.getMaxObjectSizeLimit(); - if (parentValue > 0 && result.value > 0) { - if (parentValue < result.value) { + + if (inheritProjectMaxObjectSizeLimit) { + for (ProjectState parent : parents()) { + long parentValue = parent.config.getMaxObjectSizeLimit(); + if (parentValue > 0 && result.value > 0) { + if (parentValue < result.value) { + result.value = parentValue; + result.summary = String.format(OVERRIDDEN_BY_PARENT, parent.config.getName()); + } + } else if (parentValue > 0) { result.value = parentValue; - result.summary = String.format(OVERRIDDEN_BY_PARENT, parent.config.getName()); + result.summary = String.format(INHERITED_FROM_PARENT, parent.config.getName()); } - } else if (parentValue > 0) { - result.value = parentValue; - result.summary = String.format(INHERITED_FROM_PARENT, parent.config.getName()); } } From dc118d9a7ee52abe4335cb7f02f0455009f9bc01 Mon Sep 17 00:00:00 2001 From: Jacek Centkowski Date: Fri, 31 Aug 2018 15:44:44 +0200 Subject: [PATCH 09/11] Allow more email RFC accepted chars in username MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Issue: Gerrit allows to specify email like username (especially handy when one wants to maintain unique email addresses across the server). However as opposite to what is described in [1] it allows only: ._- characters from allowed set that contains !#$%&‘*+–/=?^_`.{|}~ Considering the fact that I was able to perform clone (over SSH and HTTP) push for review, review and submit with all those characters but / this patch proposes to extend username allowed chars to !#$%&‘*+–=?^_`.{|}~ when @ is used (we have email like username) but keeps original characters set a-zA-Z0-9 when one uses single character for username. [1] https://www.mailboxvalidator.com/resources/articles/acceptable-email-address-syntax-rfc/ Change-Id: Iabb82cea5be32a68fe1d9fc65a6ca08743063a3f Signed-off-by: Jacek Centkowski --- .../gerrit/acceptance/api/accounts/AccountIT.java | 13 +++++++++++-- .../google/gerrit/client/account/UsernameField.java | 2 +- .../google/gerrit/server/account/ExternalId.java | 2 +- 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java index e5ddbbbd87..e6fd60ddc1 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java @@ -856,7 +856,13 @@ public class AccountIT extends AbstractDaemonTest { public void createUserWithValidUsername() throws Exception { ImmutableList names = ImmutableList.of( - "user@domain", "user-name", "user_name", "1234", "user1234", "1234@domain"); + "user@domain", + "user-name", + "user_name", + "1234", + "user1234", + "1234@domain", + "user!+alias{*}#$%&’^=~|@domain"); for (String name : names) { gApi.accounts().create(name); } @@ -864,7 +870,10 @@ public class AccountIT extends AbstractDaemonTest { @Test public void createUserWithInvalidUsername() throws Exception { - ImmutableList invalidNames = ImmutableList.of("@", "@foo", "-", "-foo", "_", "_foo"); + ImmutableList invalidNames = + ImmutableList.of( + "@", "@foo", "-", "-foo", "_", "_foo", "!", "+", "{", "}", "*", "%", "#", "$", "&", "’", + "^", "=", "~"); for (String name : invalidNames) { try { gApi.accounts().create(name); diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/UsernameField.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/UsernameField.java index a717a9038e..e201a8f74b 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/UsernameField.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/account/UsernameField.java @@ -41,7 +41,7 @@ class UsernameField extends Composite { // corresponding regular expressions in the // com.google.gerrit.server.account.ExternalId class. private static final String USER_NAME_PATTERN_FIRST_REGEX = "[a-zA-Z0-9]"; - private static final String USER_NAME_PATTERN_REST_REGEX = "[a-zA-Z0-9._@-]"; + private static final String USER_NAME_PATTERN_REST_REGEX = "[a-zA-Z0-9.!#$%&’*+=?^_`\\{|\\}~@-]"; private CopyableLabel userNameLbl; private NpTextBox userNameTxt; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/ExternalId.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/ExternalId.java index ad1d2fbbf2..0a8a028712 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/ExternalId.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/ExternalId.java @@ -42,7 +42,7 @@ public abstract class ExternalId implements Serializable { // corresponding regular expressions in the // com.google.gerrit.client.account.UsernameField class. private static final String USER_NAME_PATTERN_FIRST_REGEX = "[a-zA-Z0-9]"; - private static final String USER_NAME_PATTERN_REST_REGEX = "[a-zA-Z0-9._@-]"; + private static final String USER_NAME_PATTERN_REST_REGEX = "[a-zA-Z0-9.!#$%&’*+=?^_`\\{|\\}~@-]"; private static final String USER_NAME_PATTERN_LAST_REGEX = "[a-zA-Z0-9]"; /** Regular expression that a username must match. */ From 647e1f7ad8a0402a00782491a96ec98bcff06e05 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Thu, 6 Sep 2018 08:48:49 +0900 Subject: [PATCH 10/11] ElasticVersionTest#version6: Add missing test for V6_4 Change-Id: I4ca4f304cdff6d8275e60856b992ee32d9c61027 --- .../java/com/google/gerrit/elasticsearch/ElasticVersionTest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/gerrit-elasticsearch/src/test/java/com/google/gerrit/elasticsearch/ElasticVersionTest.java b/gerrit-elasticsearch/src/test/java/com/google/gerrit/elasticsearch/ElasticVersionTest.java index 9e8911422b..ec4add42ea 100644 --- a/gerrit-elasticsearch/src/test/java/com/google/gerrit/elasticsearch/ElasticVersionTest.java +++ b/gerrit-elasticsearch/src/test/java/com/google/gerrit/elasticsearch/ElasticVersionTest.java @@ -54,6 +54,7 @@ public class ElasticVersionTest { public void version6() throws Exception { assertThat(ElasticVersion.V6_2.isV6()).isTrue(); assertThat(ElasticVersion.V6_3.isV6()).isTrue(); + assertThat(ElasticVersion.V6_4.isV6()).isTrue(); assertThat(ElasticVersion.V5_6.isV6()).isFalse(); } } From 0839ebdee5656c288dd0268cd3ab97002e923892 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Thu, 6 Sep 2018 08:49:28 +0900 Subject: [PATCH 11/11] Elastic{Index|ReindexIT} Remove tests for 6.2 and 6.3 We don't need to test these versions all the time, so remove the tests. Support for versions 6.2 and 6.3 is kept; if we need to check for regressions we can manually enable the tests again later. Change-Id: Ia15785ad9cc995b4672814dd346e02a6ee4026a0 --- .../gerrit/acceptance/pgm/ElasticReindexIT.java | 12 +----------- .../google/gerrit/acceptance/ssh/ElasticIndexIT.java | 12 +----------- 2 files changed, 2 insertions(+), 22 deletions(-) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/pgm/ElasticReindexIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/pgm/ElasticReindexIT.java index 41898123d3..4e1ec08abd 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/pgm/ElasticReindexIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/pgm/ElasticReindexIT.java @@ -49,17 +49,7 @@ public class ElasticReindexIT extends AbstractReindexTests { } @ConfigSuite.Config - public static Config elasticsearchV6_2() { - return getConfig(ElasticVersion.V6_2); - } - - @ConfigSuite.Config - public static Config elasticsearchV6_3() { - return getConfig(ElasticVersion.V6_3); - } - - @ConfigSuite.Config - public static Config elasticsearchV6_4() { + public static Config elasticsearchV6() { return getConfig(ElasticVersion.V6_4); } diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/ssh/ElasticIndexIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/ssh/ElasticIndexIT.java index c77e2b2b6e..18ad621d4b 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/ssh/ElasticIndexIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/ssh/ElasticIndexIT.java @@ -46,17 +46,7 @@ public class ElasticIndexIT extends AbstractIndexTests { } @ConfigSuite.Config - public static Config elasticsearchV6_2() { - return getConfig(ElasticVersion.V6_2); - } - - @ConfigSuite.Config - public static Config elasticsearchV6_3() { - return getConfig(ElasticVersion.V6_3); - } - - @ConfigSuite.Config - public static Config elasticsearchV6_4() { + public static Config elasticsearchV6() { return getConfig(ElasticVersion.V6_4); }