Merge "Add support for cherry-picking even with merge conflicts"
diff --git a/.mailmap b/.mailmap
index c863847..b5c119c 100644
--- a/.mailmap
+++ b/.mailmap
@@ -6,6 +6,7 @@
Alex Ryazantsev <alex.ryazantsev@gmail.com> alex.ryazantsev <alex.ryazantsev@gmail.com>
Andrew Bonventre <andybons@chromium.org> <andybons@google.com>
Becky Siegel <beckysiegel@google.com> beckysiegel <beckysiegel@google.com>
+Ben Rohlfs <brohlfs@google.com> brohlfs <brohlfs@google.com>
Brad Larson <bklarson@gmail.com> <brad.larson@garmin.com>
Bruce Zu <bruce.zu.run10@gmail.com> <bruce.zu@sonyericsson.com>
Bruce Zu <bruce.zu.run10@gmail.com> <bruce.zu@sonymobile.com>
diff --git a/Documentation/dev-intellij.txt b/Documentation/dev-intellij.txt
index ca47690..b87cbf4 100644
--- a/Documentation/dev-intellij.txt
+++ b/Documentation/dev-intellij.txt
@@ -107,7 +107,9 @@
=== Copyright
Copy the folder `$(gerrit_source_code)/tools/intellij/copyright` (not just the
contents) to `$(project_data_directory)/.idea`. If it already exists, replace
-it.
+it. Then go to *File -> Settings -> Editor -> Copyright -> Copyright Profiles*,
+and import `Gerrit_Copyright.xml` to IntelliJ in case it doesn't pick the
+copyright up automatically.
=== File header
By default, IntelliJ adds a file header containing the name of the author and
diff --git a/Documentation/intro-user.txt b/Documentation/intro-user.txt
index 544039b..17c9a61 100644
--- a/Documentation/intro-user.txt
+++ b/Documentation/intro-user.txt
@@ -548,7 +548,8 @@
----
Alternatively, click *Ready* from the Change screen.
-Only change owners, project owners and site administrators can mark changes as
+Change owners, project owners, site administrators and members of a group that
+was granted "Toggle Work In Progress state" permission can mark changes as
`work-in-progress` and `ready`.
[[wip-polygerrit]]
diff --git a/Documentation/pg-plugin-endpoints.txt b/Documentation/pg-plugin-endpoints.txt
index cb1041f8..cb80c74 100644
--- a/Documentation/pg-plugin-endpoints.txt
+++ b/Documentation/pg-plugin-endpoints.txt
@@ -149,11 +149,44 @@
=== change-list-header
The `change-list-header` extension point adds a header to the change list view.
-
=== change-list-item-cell
The `change-list-item-cell` extension point adds a cell to the change list item.
+In addition to default parameters, the following are available:
+
* `change`
+
current change of the row, an instance of
link:rest-api-changes.html#change-info[ChangeInfo]
+
+=== change-view-tab-header
+The `change-view-tab-header` extension point adds a primary tab to the change
+view. This must be used in conjunction with `change-view-tab-content`.
+
+In addition to default parameters, the following are available:
+
+* `change`
++
+current change displayed, an instance of
+link:rest-api-changes.html#change-info[ChangeInfo]
+
+* `revision`
++
+current revision displayed, an instance of
+link:rest-api-changes.html#revision-info[RevisionInfo]
+
+=== change-view-tab-content
+The `change-view-tab-content` extension point adds primary tab content to
+the change view. This must be used in conjunction with `change-view-tab-header`.
+
+In addition to default parameters, the following are available:
+
+* `change`
++
+current change displayed, an instance of
+link:rest-api-changes.html#change-info[ChangeInfo]
+
+* `revision`
++
+current revision displayed, an instance of
+link:rest-api-changes.html#revision-info[RevisionInfo]
diff --git a/Documentation/user-search.txt b/Documentation/user-search.txt
index 5d7a78b..cafd5ca 100644
--- a/Documentation/user-search.txt
+++ b/Documentation/user-search.txt
@@ -216,7 +216,8 @@
[[hashtag]]
hashtag:'HASHTAG'::
+
-Changes whose link:intro-user.html#hashtags[hashtag] matches 'HASHTAG' exactly.
+Changes whose link:intro-user.html#hashtags[hashtag] matches 'HASHTAG'.
+The match is case-insensitive.
[[ref]]
ref:'REF'::
diff --git a/WORKSPACE b/WORKSPACE
index 7aa4dce..c2629ba 100644
--- a/WORKSPACE
+++ b/WORKSPACE
@@ -47,11 +47,11 @@
# Golang support for PolyGerrit local dev server.
http_archive(
name = "io_bazel_rules_go",
- sha256 = "ee5fe78fe417c685ecb77a0a725dc9f6040ae5beb44a0ba4ddb55453aad23a8a",
- url = "https://github.com/bazelbuild/rules_go/releases/download/0.16.0/rules_go-0.16.0.tar.gz",
+ sha256 = "6776d68ebb897625dead17ae510eac3d5f6342367327875210df44dbe2aeeb19",
+ url = "https://github.com/bazelbuild/rules_go/releases/download/0.17.1/rules_go-0.17.1.tar.gz",
)
-load("@io_bazel_rules_go//go:def.bzl", "go_register_toolchains", "go_rules_dependencies")
+load("@io_bazel_rules_go//go:deps.bzl", "go_register_toolchains", "go_rules_dependencies")
go_rules_dependencies()
@@ -59,8 +59,8 @@
http_archive(
name = "bazel_gazelle",
- sha256 = "c0a5739d12c6d05b6c1ad56f2200cb0b57c5a70e03ebd2f7b87ce88cabf09c7b",
- urls = ["https://github.com/bazelbuild/bazel-gazelle/releases/download/0.14.0/bazel-gazelle-0.14.0.tar.gz"],
+ sha256 = "3c681998538231a2d24d0c07ed5a7658cb72bfb5fd4bf9911157c0e9ac6a2687",
+ urls = ["https://github.com/bazelbuild/bazel-gazelle/releases/download/0.17.0/bazel-gazelle-0.17.0.tar.gz"],
)
load("@bazel_gazelle//:deps.bzl", "gazelle_dependencies", "go_repository")
diff --git a/java/com/google/gerrit/common/data/Permission.java b/java/com/google/gerrit/common/data/Permission.java
index 2e9c2d6..3ba0ba7 100644
--- a/java/com/google/gerrit/common/data/Permission.java
+++ b/java/com/google/gerrit/common/data/Permission.java
@@ -46,6 +46,7 @@
public static final String REMOVE_REVIEWER = "removeReviewer";
public static final String SUBMIT = "submit";
public static final String SUBMIT_AS = "submitAs";
+ public static final String TOGGLE_WORK_IN_PROGRESS_STATE = "toggleWipState";
public static final String VIEW_PRIVATE_CHANGES = "viewPrivateChanges";
private static final List<String> NAMES_LC;
@@ -78,6 +79,7 @@
NAMES_LC.add(REMOVE_REVIEWER.toLowerCase());
NAMES_LC.add(SUBMIT.toLowerCase());
NAMES_LC.add(SUBMIT_AS.toLowerCase());
+ NAMES_LC.add(TOGGLE_WORK_IN_PROGRESS_STATE.toLowerCase());
NAMES_LC.add(VIEW_PRIVATE_CHANGES.toLowerCase());
LABEL_INDEX = NAMES_LC.indexOf(Permission.LABEL);
diff --git a/java/com/google/gerrit/extensions/config/CapabilityDefinition.java b/java/com/google/gerrit/extensions/config/CapabilityDefinition.java
index aafb583..c76ec6c 100644
--- a/java/com/google/gerrit/extensions/config/CapabilityDefinition.java
+++ b/java/com/google/gerrit/extensions/config/CapabilityDefinition.java
@@ -18,7 +18,4 @@
/** Specifies a capability declared by a plugin. */
@ExtensionPoint
-public abstract class CapabilityDefinition {
- /** @return description of the capability. */
- public abstract String getDescription();
-}
+public abstract class CapabilityDefinition implements PluginPermissionDefinition {}
diff --git a/java/com/google/gerrit/extensions/config/PluginPermissionDefinition.java b/java/com/google/gerrit/extensions/config/PluginPermissionDefinition.java
new file mode 100644
index 0000000..11b1981
--- /dev/null
+++ b/java/com/google/gerrit/extensions/config/PluginPermissionDefinition.java
@@ -0,0 +1,25 @@
+// 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.extensions.config;
+
+/** Specifies a permission declared by a plugin. */
+public interface PluginPermissionDefinition {
+ /**
+ * Gets the description of a permission declared by a plugin.
+ *
+ * @return description of the permission.
+ */
+ String getDescription();
+}
diff --git a/java/com/google/gerrit/extensions/config/PluginProjectPermissionDefinition.java b/java/com/google/gerrit/extensions/config/PluginProjectPermissionDefinition.java
new file mode 100644
index 0000000..d1d9f9e
--- /dev/null
+++ b/java/com/google/gerrit/extensions/config/PluginProjectPermissionDefinition.java
@@ -0,0 +1,21 @@
+// 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.extensions.config;
+
+import com.google.gerrit.extensions.annotations.ExtensionPoint;
+
+/** Specifies a repository permission declared by a plugin. */
+@ExtensionPoint
+public abstract class PluginProjectPermissionDefinition implements PluginPermissionDefinition {}
diff --git a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
index 2b00d7c..d6dca48 100644
--- a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
+++ b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
@@ -392,8 +392,12 @@
if (isRead(req)) {
viewData = new ViewData(null, c.list());
} else if (isPost(req)) {
+ // TODO: Here and on other collection methods: There is a bug that binds child views
+ // with pluginName="gerrit" instead of the real plugin name. This has never worked
+ // correctly and should be fixed where the binding gets created (DynamicMapProvider)
+ // and here.
RestView<RestResource> restCollectionView =
- c.views().get(viewData.pluginName, "POST_ON_COLLECTION./");
+ c.views().get(PluginName.GERRIT, "POST_ON_COLLECTION./");
if (restCollectionView != null) {
viewData = new ViewData(null, restCollectionView);
} else {
@@ -401,7 +405,7 @@
}
} else if (isDelete(req)) {
RestView<RestResource> restCollectionView =
- c.views().get(viewData.pluginName, "DELETE_ON_COLLECTION./");
+ c.views().get(PluginName.GERRIT, "DELETE_ON_COLLECTION./");
if (restCollectionView != null) {
viewData = new ViewData(null, restCollectionView);
} else {
@@ -1264,6 +1268,8 @@
return new ViewData(PluginName.GERRIT, core);
}
+ // Check if we want to delegate to a child collection. Child collections are bound with
+ // GET.name so we have to check for this since we haven't found any other views.
core = views.get(PluginName.GERRIT, "GET." + p.get(0));
if (core != null) {
return new ViewData(PluginName.GERRIT, core);
@@ -1277,6 +1283,17 @@
}
}
+ if (r.isEmpty()) {
+ // Check if we want to delegate to a child collection. Child collections are bound with
+ // GET.name so we have to check for this since we haven't found any other views.
+ for (String plugin : views.plugins()) {
+ RestView<RestResource> action = views.get(plugin, "GET." + p.get(0));
+ if (action != null) {
+ r.put(plugin, action);
+ }
+ }
+ }
+
if (r.size() == 1) {
Map.Entry<String, RestView<RestResource>> entry = Iterables.getOnlyElement(r.entrySet());
return new ViewData(entry.getKey(), entry.getValue());
diff --git a/java/com/google/gerrit/server/change/WorkInProgressOp.java b/java/com/google/gerrit/server/change/WorkInProgressOp.java
index 02870fb..e5fdd8f 100644
--- a/java/com/google/gerrit/server/change/WorkInProgressOp.java
+++ b/java/com/google/gerrit/server/change/WorkInProgressOp.java
@@ -18,20 +18,14 @@
import com.google.common.collect.ImmutableList;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.extensions.api.changes.NotifyHandling;
-import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.ChangeMessage;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.server.ChangeMessagesUtil;
-import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.extensions.events.WorkInProgressStateChanged;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ChangeUpdate;
-import com.google.gerrit.server.permissions.GlobalPermission;
-import com.google.gerrit.server.permissions.PermissionBackend;
-import com.google.gerrit.server.permissions.PermissionBackendException;
-import com.google.gerrit.server.permissions.ProjectPermission;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gerrit.server.update.Context;
@@ -57,34 +51,6 @@
WorkInProgressOp create(boolean workInProgress, Input in);
}
- public static void checkPermissions(
- PermissionBackend permissionBackend, CurrentUser user, Change change)
- throws PermissionBackendException, AuthException {
- if (!user.isIdentifiedUser()) {
- throw new AuthException("Authentication required");
- }
-
- if (change.getOwner().equals(user.asIdentifiedUser().getAccountId())) {
- return;
- }
-
- try {
- permissionBackend.currentUser().check(GlobalPermission.ADMINISTRATE_SERVER);
- return;
- } catch (AuthException e) {
- // Skip.
- }
-
- try {
- permissionBackend
- .user(user)
- .project(change.getProject())
- .check(ProjectPermission.WRITE_CONFIG);
- } catch (AuthException exp) {
- throw new AuthException("not allowed to toggle work in progress");
- }
- }
-
private final ChangeMessagesUtil cmUtil;
private final EmailReviewComments.Factory email;
private final PatchSetUtil psUtil;
diff --git a/java/com/google/gerrit/server/config/GerritGlobalModule.java b/java/com/google/gerrit/server/config/GerritGlobalModule.java
index 4158346..711efaf 100644
--- a/java/com/google/gerrit/server/config/GerritGlobalModule.java
+++ b/java/com/google/gerrit/server/config/GerritGlobalModule.java
@@ -29,6 +29,7 @@
import com.google.gerrit.extensions.config.DownloadScheme;
import com.google.gerrit.extensions.config.ExternalIncludedIn;
import com.google.gerrit.extensions.config.FactoryModule;
+import com.google.gerrit.extensions.config.PluginProjectPermissionDefinition;
import com.google.gerrit.extensions.events.AccountIndexedListener;
import com.google.gerrit.extensions.events.AgreementSignupListener;
import com.google.gerrit.extensions.events.AssigneeChangedListener;
@@ -309,6 +310,7 @@
DynamicMap.mapOf(binder(), new TypeLiteral<Cache<?, ?>>() {});
DynamicSet.setOf(binder(), CacheRemovalListener.class);
DynamicMap.mapOf(binder(), CapabilityDefinition.class);
+ DynamicMap.mapOf(binder(), PluginProjectPermissionDefinition.class);
DynamicSet.setOf(binder(), GitReferenceUpdatedListener.class);
DynamicSet.setOf(binder(), AssigneeChangedListener.class);
DynamicSet.setOf(binder(), ChangeAbandonedListener.class);
diff --git a/java/com/google/gerrit/server/git/receive/AsyncReceiveCommits.java b/java/com/google/gerrit/server/git/receive/AsyncReceiveCommits.java
index f9fb251..f0077cd 100644
--- a/java/com/google/gerrit/server/git/receive/AsyncReceiveCommits.java
+++ b/java/com/google/gerrit/server/git/receive/AsyncReceiveCommits.java
@@ -351,6 +351,7 @@
} else {
List<Change.Id> autoclosed = resultChangeIds.get(ResultChangeIds.Key.AUTOCLOSED);
metrics.changes.record(ResultChangeIds.Key.AUTOCLOSED, autoclosed.size());
+ totalChanges += autoclosed.size();
}
if (totalChanges > 0) {
diff --git a/java/com/google/gerrit/server/permissions/ChangeControl.java b/java/com/google/gerrit/server/permissions/ChangeControl.java
index 250ca03..309bf51 100644
--- a/java/com/google/gerrit/server/permissions/ChangeControl.java
+++ b/java/com/google/gerrit/server/permissions/ChangeControl.java
@@ -176,6 +176,14 @@
return refControl.canForceEditTopicName();
}
+ /** Can this user toggle WorkInProgress state? */
+ private boolean canToggleWorkInProgressState() {
+ return isOwner()
+ || getProjectControl().isOwner()
+ || refControl.canPerform(Permission.TOGGLE_WORK_IN_PROGRESS_STATE)
+ || getProjectControl().isAdmin();
+ }
+
/** Can this user edit the description? */
private boolean canEditDescription() {
if (getChange().isNew()) {
@@ -299,6 +307,8 @@
return canRestore();
case SUBMIT:
return refControl.canSubmit(isOwner());
+ case TOGGLE_WORK_IN_PROGRESS_STATE:
+ return canToggleWorkInProgressState();
case REMOVE_REVIEWER:
case SUBMIT_AS:
diff --git a/java/com/google/gerrit/server/permissions/ChangePermission.java b/java/com/google/gerrit/server/permissions/ChangePermission.java
index ca1c460..2fba4ef 100644
--- a/java/com/google/gerrit/server/permissions/ChangePermission.java
+++ b/java/com/google/gerrit/server/permissions/ChangePermission.java
@@ -55,7 +55,8 @@
*/
REBASE,
SUBMIT,
- SUBMIT_AS("submit on behalf of other users");
+ SUBMIT_AS("submit on behalf of other users"),
+ TOGGLE_WORK_IN_PROGRESS_STATE;
private final String description;
diff --git a/java/com/google/gerrit/server/permissions/DefaultPermissionMappings.java b/java/com/google/gerrit/server/permissions/DefaultPermissionMappings.java
index ece29df..d56bc78 100644
--- a/java/com/google/gerrit/server/permissions/DefaultPermissionMappings.java
+++ b/java/com/google/gerrit/server/permissions/DefaultPermissionMappings.java
@@ -97,6 +97,9 @@
.put(ChangePermission.REBASE, Permission.REBASE)
.put(ChangePermission.SUBMIT, Permission.SUBMIT)
.put(ChangePermission.SUBMIT_AS, Permission.SUBMIT_AS)
+ .put(
+ ChangePermission.TOGGLE_WORK_IN_PROGRESS_STATE,
+ Permission.TOGGLE_WORK_IN_PROGRESS_STATE)
.build();
private static <T extends Enum<T>> void checkMapContainsAllEnumValues(
diff --git a/java/com/google/gerrit/server/permissions/PluginPermissionsUtil.java b/java/com/google/gerrit/server/permissions/PluginPermissionsUtil.java
new file mode 100644
index 0000000..a837ed7
--- /dev/null
+++ b/java/com/google/gerrit/server/permissions/PluginPermissionsUtil.java
@@ -0,0 +1,110 @@
+// 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.server.permissions;
+
+import com.google.common.collect.ImmutableMap;
+import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.extensions.config.CapabilityDefinition;
+import com.google.gerrit.extensions.config.PluginPermissionDefinition;
+import com.google.gerrit.extensions.config.PluginProjectPermissionDefinition;
+import com.google.gerrit.extensions.registration.DynamicMap;
+import com.google.gerrit.extensions.registration.Extension;
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+import java.util.regex.Pattern;
+
+/** Utilities for plugin permissions. */
+@Singleton
+public final class PluginPermissionsUtil {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
+ private static final String PLUGIN_NAME_PATTERN_STRING = "[a-zA-Z0-9-]+";
+
+ /**
+ * Name pattern for a plugin non-capability permission stored in the config file.
+ *
+ * <p>This pattern requires a plugin declared permission to have a name in the access section of
+ * {@code ProjectConfig} with a format like "plugin-{pluginName}-{permissionName}", which makes it
+ * easier to tell if a config name represents a plugin permission or not. Note "-" isn't clear
+ * enough for this purpose since some core permissions, e.g. "label-", also contain "-".
+ */
+ private static final Pattern PLUGIN_PERMISSION_NAME_IN_CONFIG_PATTERN =
+ Pattern.compile("^plugin-" + PLUGIN_NAME_PATTERN_STRING + "-[a-zA-Z]+$");
+
+ /** Name pattern for a Gerrit plugin. */
+ private static final Pattern PLUGIN_NAME_PATTERN =
+ Pattern.compile("^" + PLUGIN_NAME_PATTERN_STRING + "$");
+
+ private final DynamicMap<CapabilityDefinition> capabilityDefinitions;
+ private final DynamicMap<PluginProjectPermissionDefinition> pluginProjectPermissionDefinitions;
+
+ @Inject
+ PluginPermissionsUtil(
+ DynamicMap<CapabilityDefinition> capabilityDefinitions,
+ DynamicMap<PluginProjectPermissionDefinition> pluginProjectPermissionDefinitions) {
+ this.capabilityDefinitions = capabilityDefinitions;
+ this.pluginProjectPermissionDefinitions = pluginProjectPermissionDefinitions;
+ }
+
+ /**
+ * Collects all the plugin declared capabilities.
+ *
+ * @return a map of plugin declared capabilities with "pluginName" as its keys and
+ * "pluginName-{permissionName}" as its values.
+ */
+ public ImmutableMap<String, String> collectPluginCapabilities() {
+ return collectPermissions(capabilityDefinitions, "");
+ }
+
+ /**
+ * Collects all the plugin declared project permissions.
+ *
+ * @return a map of plugin declared project permissions with "{pluginName}" as its keys and
+ * "plugin-{pluginName}-{permissionName}" as its values.
+ */
+ public ImmutableMap<String, String> collectPluginProjectPermissions() {
+ return collectPermissions(pluginProjectPermissionDefinitions, "plugin-");
+ }
+
+ private static <T extends PluginPermissionDefinition>
+ ImmutableMap<String, String> collectPermissions(DynamicMap<T> definitions, String prefix) {
+ ImmutableMap.Builder<String, String> permissionIdNames = ImmutableMap.builder();
+
+ for (Extension<T> extension : definitions) {
+ String pluginName = extension.getPluginName();
+ if (!PLUGIN_NAME_PATTERN.matcher(pluginName).matches()) {
+ logger.atWarning().log(
+ "Plugin name '%s' must match '%s' to use permissions; rename the plugin",
+ pluginName, PLUGIN_NAME_PATTERN.pattern());
+ continue;
+ }
+
+ String id = prefix + pluginName + "-" + extension.getExportName();
+ permissionIdNames.put(id, extension.get().getDescription());
+ }
+
+ return permissionIdNames.build();
+ }
+
+ /**
+ * Checks if a given name matches the plugin declared permission name pattern for configs.
+ *
+ * @param name a config name which may stand for a plugin permission.
+ * @return whether the name matches the plugin permission name pattern for configs.
+ */
+ public static boolean isPluginPermission(String name) {
+ return PLUGIN_PERMISSION_NAME_IN_CONFIG_PATTERN.matcher(name).matches();
+ }
+}
diff --git a/java/com/google/gerrit/server/project/ProjectConfig.java b/java/com/google/gerrit/server/project/ProjectConfig.java
index c4ef32e..439e7c0 100644
--- a/java/com/google/gerrit/server/project/ProjectConfig.java
+++ b/java/com/google/gerrit/server/project/ProjectConfig.java
@@ -18,6 +18,7 @@
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.gerrit.common.data.Permission.isPermission;
import static com.google.gerrit.reviewdb.client.Project.DEFAULT_SUBMIT_TYPE;
+import static com.google.gerrit.server.permissions.PluginPermissionsUtil.isPluginPermission;
import static java.util.stream.Collectors.toList;
import com.google.common.base.CharMatcher;
@@ -747,7 +748,7 @@
for (String varName : rc.getStringList(ACCESS, refName, KEY_GROUP_PERMISSIONS)) {
for (String n : Splitter.on(EXCLUSIVE_PERMISSIONS_SPLIT_PATTERN).split(varName)) {
n = convertLegacyPermission(n);
- if (isPermission(n)) {
+ if (isCoreOrPluginPermission(n)) {
as.getPermission(n, true).setExclusiveGroup(true);
}
}
@@ -755,7 +756,7 @@
for (String varName : rc.getNames(ACCESS, refName)) {
String convertedName = convertLegacyPermission(varName);
- if (isPermission(convertedName)) {
+ if (isCoreOrPluginPermission(convertedName)) {
Permission perm = as.getPermission(convertedName, true);
loadPermissionRules(
rc,
@@ -784,6 +785,12 @@
}
}
+ private boolean isCoreOrPluginPermission(String permission) {
+ // Since plugins are loaded dynamically, here we can't load all plugin permissions and verify
+ // their existence.
+ return isPermission(permission) || isPluginPermission(permission);
+ }
+
private boolean isValidRegex(String refPattern) {
try {
RefPattern.validateRegExp(refPattern);
@@ -1360,7 +1367,7 @@
}
for (String varName : rc.getNames(ACCESS, refName)) {
- if (isPermission(convertLegacyPermission(varName))
+ if (isCoreOrPluginPermission(convertLegacyPermission(varName))
&& !have.contains(varName.toLowerCase())) {
rc.unset(ACCESS, refName, varName);
}
diff --git a/java/com/google/gerrit/server/query/change/ChangeIndexPredicate.java b/java/com/google/gerrit/server/query/change/ChangeIndexPredicate.java
index 1eb2770..7428e3a 100644
--- a/java/com/google/gerrit/server/query/change/ChangeIndexPredicate.java
+++ b/java/com/google/gerrit/server/query/change/ChangeIndexPredicate.java
@@ -17,9 +17,22 @@
import com.google.gerrit.index.FieldDef;
import com.google.gerrit.index.query.IndexPredicate;
import com.google.gerrit.index.query.Matchable;
+import com.google.gerrit.index.query.Predicate;
public abstract class ChangeIndexPredicate extends IndexPredicate<ChangeData>
implements Matchable<ChangeData> {
+ /**
+ * Returns an index predicate that matches no changes in the index.
+ *
+ * <p>This predicate should be used in preference to a non-index predicate (such as {@code
+ * Predicate.not(Predicate.any())}), since it can be matched efficiently against the index.
+ *
+ * @return an index predicate matching no changes.
+ */
+ public static Predicate<ChangeData> none() {
+ return ChangeStatusPredicate.NONE;
+ }
+
protected ChangeIndexPredicate(FieldDef<ChangeData, ?> def, String value) {
super(def, value);
}
diff --git a/java/com/google/gerrit/server/query/change/ChangeStatusPredicate.java b/java/com/google/gerrit/server/query/change/ChangeStatusPredicate.java
index 8dc17d3..5a7efb8 100644
--- a/java/com/google/gerrit/server/query/change/ChangeStatusPredicate.java
+++ b/java/com/google/gerrit/server/query/change/ChangeStatusPredicate.java
@@ -41,7 +41,7 @@
*/
public final class ChangeStatusPredicate extends ChangeIndexPredicate {
private static final String INVALID_STATUS = "__invalid__";
- private static final Predicate<ChangeData> NONE = new ChangeStatusPredicate(null);
+ static final Predicate<ChangeData> NONE = new ChangeStatusPredicate(null);
private static final TreeMap<String, Predicate<ChangeData>> PREDICATES;
private static final Predicate<ChangeData> CLOSED;
diff --git a/java/com/google/gerrit/server/query/change/ConflictsPredicate.java b/java/com/google/gerrit/server/query/change/ConflictsPredicate.java
index 3fc512a..ada9f27 100644
--- a/java/com/google/gerrit/server/query/change/ConflictsPredicate.java
+++ b/java/com/google/gerrit/server/query/change/ConflictsPredicate.java
@@ -19,7 +19,6 @@
import static java.util.concurrent.TimeUnit.MINUTES;
import com.google.common.flogger.FluentLogger;
-import com.google.common.flogger.LoggingApi;
import com.google.gerrit.common.data.SubmitTypeRecord;
import com.google.gerrit.index.query.PostFilterPredicate;
import com.google.gerrit.index.query.Predicate;
@@ -42,8 +41,6 @@
import java.util.HashSet;
import java.util.List;
import java.util.Set;
-import java.util.function.BiConsumer;
-import java.util.function.Supplier;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
@@ -65,13 +62,12 @@
cd = args.changeDataFactory.create(c);
files = cd.currentFilePaths();
} catch (IOException | OrmException e) {
- logWithOccasionalStackTrace(
- logger::atWarning,
+ warnWithOccasionalStackTrace(
e,
"Error constructing conflicts predicates for change %s in %s",
c.getId(),
c.getProject());
- return Predicate.not(Predicate.any());
+ return ChangeIndexPredicate.none();
}
if (3 + files.size() > args.indexConfig.maxTerms()) {
@@ -164,8 +160,7 @@
}
} catch (IntegrationException | NoSuchProjectException | OrmException | IOException e) {
ObjectId finalOther = other;
- logWithOccasionalStackTrace(
- logger::atWarning,
+ warnWithOccasionalStackTrace(
e,
"Merge failure checking conflicts of change %s in %s (%s): %s",
id,
@@ -235,12 +230,12 @@
}
}
- private static <API extends LoggingApi<API>> void logWithOccasionalStackTrace(
- Supplier<LoggingApi<API>> logSupplier, Throwable cause, String format, Object... args) {
- BiConsumer<LoggingApi<API>, String> logConsumer = (api, f) -> api.logVarargs(f, args);
- logConsumer.accept(logSupplier.get(), format);
- logConsumer.accept(
- logSupplier.get().withCause(cause).atMostEvery(1, MINUTES),
- "(Re-logging with stack trace) " + format);
+ private static void warnWithOccasionalStackTrace(Throwable cause, String format, Object... args) {
+ logger.atWarning().logVarargs(format, args);
+ logger
+ .atWarning()
+ .withCause(cause)
+ .atMostEvery(1, MINUTES)
+ .logVarargs("(Re-logging with stack trace) " + format, args);
}
}
diff --git a/java/com/google/gerrit/server/restapi/change/PostReview.java b/java/com/google/gerrit/server/restapi/change/PostReview.java
index 8afaa45..ff27bfa 100644
--- a/java/com/google/gerrit/server/restapi/change/PostReview.java
+++ b/java/com/google/gerrit/server/restapi/change/PostReview.java
@@ -340,8 +340,10 @@
return Response.withStatusCode(SC_BAD_REQUEST, output);
}
- WorkInProgressOp.checkPermissions(
- permissionBackend, revision.getUser(), revision.getChange());
+ revision
+ .getChangeResource()
+ .permissions()
+ .check(ChangePermission.TOGGLE_WORK_IN_PROGRESS_STATE);
if (input.ready) {
output.ready = true;
diff --git a/java/com/google/gerrit/server/restapi/change/SetReadyForReview.java b/java/com/google/gerrit/server/restapi/change/SetReadyForReview.java
index 94914b0..aacf58b 100644
--- a/java/com/google/gerrit/server/restapi/change/SetReadyForReview.java
+++ b/java/com/google/gerrit/server/restapi/change/SetReadyForReview.java
@@ -16,7 +16,6 @@
import static com.google.common.base.MoreObjects.firstNonNull;
import static com.google.gerrit.extensions.conditions.BooleanCondition.and;
-import static com.google.gerrit.extensions.conditions.BooleanCondition.or;
import com.google.gerrit.extensions.api.changes.NotifyHandling;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
@@ -25,48 +24,36 @@
import com.google.gerrit.extensions.webui.UiAction;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.server.ChangeUtil;
-import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.change.NotifyResolver;
import com.google.gerrit.server.change.WorkInProgressOp;
import com.google.gerrit.server.change.WorkInProgressOp.Input;
-import com.google.gerrit.server.permissions.GlobalPermission;
-import com.google.gerrit.server.permissions.PermissionBackend;
+import com.google.gerrit.server.permissions.ChangePermission;
import com.google.gerrit.server.permissions.PermissionBackendException;
-import com.google.gerrit.server.permissions.ProjectPermission;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.RetryHelper;
import com.google.gerrit.server.update.RetryingRestModifyView;
import com.google.gerrit.server.update.UpdateException;
import com.google.gerrit.server.util.time.TimeUtil;
import com.google.inject.Inject;
-import com.google.inject.Provider;
import com.google.inject.Singleton;
@Singleton
public class SetReadyForReview extends RetryingRestModifyView<ChangeResource, Input, Response<?>>
implements UiAction<ChangeResource> {
private final WorkInProgressOp.Factory opFactory;
- private final PermissionBackend permissionBackend;
- private final Provider<CurrentUser> user;
@Inject
- SetReadyForReview(
- RetryHelper retryHelper,
- WorkInProgressOp.Factory opFactory,
- PermissionBackend permissionBackend,
- Provider<CurrentUser> user) {
+ SetReadyForReview(RetryHelper retryHelper, WorkInProgressOp.Factory opFactory) {
super(retryHelper);
this.opFactory = opFactory;
- this.permissionBackend = permissionBackend;
- this.user = user;
}
@Override
protected Response<?> applyImpl(
BatchUpdate.Factory updateFactory, ChangeResource rsrc, Input input)
throws RestApiException, UpdateException, PermissionBackendException {
- WorkInProgressOp.checkPermissions(permissionBackend, user.get(), rsrc.getChange());
+ rsrc.permissions().check(ChangePermission.TOGGLE_WORK_IN_PROGRESS_STATE);
Change change = rsrc.getChange();
if (!change.isNew()) {
@@ -94,15 +81,6 @@
.setVisible(
and(
rsrc.getChange().isNew() && rsrc.getChange().isWorkInProgress(),
- or(
- rsrc.isUserOwner(),
- or(
- permissionBackend
- .currentUser()
- .testCond(GlobalPermission.ADMINISTRATE_SERVER),
- permissionBackend
- .currentUser()
- .project(rsrc.getProject())
- .testCond(ProjectPermission.WRITE_CONFIG)))));
+ rsrc.permissions().testCond(ChangePermission.TOGGLE_WORK_IN_PROGRESS_STATE)));
}
}
diff --git a/java/com/google/gerrit/server/restapi/change/SetWorkInProgress.java b/java/com/google/gerrit/server/restapi/change/SetWorkInProgress.java
index d087e47..852813e 100644
--- a/java/com/google/gerrit/server/restapi/change/SetWorkInProgress.java
+++ b/java/com/google/gerrit/server/restapi/change/SetWorkInProgress.java
@@ -16,7 +16,6 @@
import static com.google.common.base.MoreObjects.firstNonNull;
import static com.google.gerrit.extensions.conditions.BooleanCondition.and;
-import static com.google.gerrit.extensions.conditions.BooleanCondition.or;
import com.google.gerrit.extensions.api.changes.NotifyHandling;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
@@ -25,48 +24,36 @@
import com.google.gerrit.extensions.webui.UiAction;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.server.ChangeUtil;
-import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.change.NotifyResolver;
import com.google.gerrit.server.change.WorkInProgressOp;
import com.google.gerrit.server.change.WorkInProgressOp.Input;
-import com.google.gerrit.server.permissions.GlobalPermission;
-import com.google.gerrit.server.permissions.PermissionBackend;
+import com.google.gerrit.server.permissions.ChangePermission;
import com.google.gerrit.server.permissions.PermissionBackendException;
-import com.google.gerrit.server.permissions.ProjectPermission;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.RetryHelper;
import com.google.gerrit.server.update.RetryingRestModifyView;
import com.google.gerrit.server.update.UpdateException;
import com.google.gerrit.server.util.time.TimeUtil;
import com.google.inject.Inject;
-import com.google.inject.Provider;
import com.google.inject.Singleton;
@Singleton
public class SetWorkInProgress extends RetryingRestModifyView<ChangeResource, Input, Response<?>>
implements UiAction<ChangeResource> {
private final WorkInProgressOp.Factory opFactory;
- private final PermissionBackend permissionBackend;
- private final Provider<CurrentUser> user;
@Inject
- SetWorkInProgress(
- WorkInProgressOp.Factory opFactory,
- RetryHelper retryHelper,
- PermissionBackend permissionBackend,
- Provider<CurrentUser> user) {
+ SetWorkInProgress(WorkInProgressOp.Factory opFactory, RetryHelper retryHelper) {
super(retryHelper);
this.opFactory = opFactory;
- this.permissionBackend = permissionBackend;
- this.user = user;
}
@Override
protected Response<?> applyImpl(
BatchUpdate.Factory updateFactory, ChangeResource rsrc, Input input)
throws RestApiException, UpdateException, PermissionBackendException {
- WorkInProgressOp.checkPermissions(permissionBackend, user.get(), rsrc.getChange());
+ rsrc.permissions().check(ChangePermission.TOGGLE_WORK_IN_PROGRESS_STATE);
Change change = rsrc.getChange();
if (!change.isNew()) {
@@ -94,15 +81,6 @@
.setVisible(
and(
rsrc.getChange().isNew() && !rsrc.getChange().isWorkInProgress(),
- or(
- rsrc.isUserOwner(),
- or(
- permissionBackend
- .currentUser()
- .testCond(GlobalPermission.ADMINISTRATE_SERVER),
- permissionBackend
- .currentUser()
- .project(rsrc.getProject())
- .testCond(ProjectPermission.WRITE_CONFIG)))));
+ rsrc.permissions().testCond(ChangePermission.TOGGLE_WORK_IN_PROGRESS_STATE)));
}
}
diff --git a/java/com/google/gerrit/server/restapi/config/ListCapabilities.java b/java/com/google/gerrit/server/restapi/config/ListCapabilities.java
index fa9bfde..9bb2e6d 100644
--- a/java/com/google/gerrit/server/restapi/config/ListCapabilities.java
+++ b/java/com/google/gerrit/server/restapi/config/ListCapabilities.java
@@ -14,38 +14,32 @@
package com.google.gerrit.server.restapi.config;
+import static com.google.common.collect.ImmutableMap.toImmutableMap;
+
import com.google.common.collect.ImmutableMap;
-import com.google.common.flogger.FluentLogger;
import com.google.gerrit.common.data.GlobalCapability;
-import com.google.gerrit.extensions.config.CapabilityDefinition;
-import com.google.gerrit.extensions.registration.DynamicMap;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.RestReadView;
import com.google.gerrit.server.config.CapabilityConstants;
import com.google.gerrit.server.config.ConfigResource;
import com.google.gerrit.server.permissions.PermissionBackend;
+import com.google.gerrit.server.permissions.PluginPermissionsUtil;
import com.google.inject.Inject;
-import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.util.HashMap;
import java.util.Map;
-import java.util.regex.Pattern;
/** List capabilities visible to the calling user. */
@Singleton
public class ListCapabilities implements RestReadView<ConfigResource> {
- private static final FluentLogger logger = FluentLogger.forEnclosingClass();
-
- private static final Pattern PLUGIN_NAME_PATTERN = Pattern.compile("^[a-zA-Z0-9-]+$");
-
private final PermissionBackend permissionBackend;
- private final DynamicMap<CapabilityDefinition> pluginCapabilities;
+ private final PluginPermissionsUtil pluginPermissionsUtil;
@Inject
public ListCapabilities(
- PermissionBackend permissionBackend, DynamicMap<CapabilityDefinition> pluginCapabilities) {
+ PermissionBackend permissionBackend, PluginPermissionsUtil pluginPermissionsUtil) {
this.permissionBackend = permissionBackend;
- this.pluginCapabilities = pluginCapabilities;
+ this.pluginPermissionsUtil = pluginPermissionsUtil;
}
@Override
@@ -59,21 +53,16 @@
}
public Map<String, CapabilityInfo> collectPluginCapabilities() {
- Map<String, CapabilityInfo> output = new HashMap<>();
- for (String pluginName : pluginCapabilities.plugins()) {
- if (!PLUGIN_NAME_PATTERN.matcher(pluginName).matches()) {
- logger.atWarning().log(
- "Plugin name '%s' must match '%s' to use capabilities; rename the plugin",
- pluginName, PLUGIN_NAME_PATTERN.pattern());
- continue;
- }
- for (Map.Entry<String, Provider<CapabilityDefinition>> entry :
- pluginCapabilities.byPlugin(pluginName).entrySet()) {
- String id = String.format("%s-%s", pluginName, entry.getKey());
- output.put(id, new CapabilityInfo(id, entry.getValue().get().getDescription()));
- }
- }
- return output;
+ return convertToPermissionInfos(pluginPermissionsUtil.collectPluginCapabilities());
+ }
+
+ private static ImmutableMap<String, CapabilityInfo> convertToPermissionInfos(
+ ImmutableMap<String, String> permissionIdNames) {
+ return permissionIdNames
+ .entrySet()
+ .stream()
+ .collect(
+ toImmutableMap(Map.Entry::getKey, e -> new CapabilityInfo(e.getKey(), e.getValue())));
}
private Map<String, CapabilityInfo> collectCoreCapabilities()
diff --git a/java/com/google/gerrit/server/util/CommitMessageUtil.java b/java/com/google/gerrit/server/util/CommitMessageUtil.java
index fa55597..e984f46 100644
--- a/java/com/google/gerrit/server/util/CommitMessageUtil.java
+++ b/java/com/google/gerrit/server/util/CommitMessageUtil.java
@@ -15,6 +15,7 @@
package com.google.gerrit.server.util;
import com.google.common.base.Strings;
+import com.google.gerrit.common.Nullable;
import com.google.gerrit.extensions.restapi.BadRequestException;
/** Utility functions to manipulate commit messages. */
@@ -23,18 +24,22 @@
private CommitMessageUtil() {}
/**
- * Checks for null or empty commit messages and appends a newline character to the commit message.
+ * Checks for invalid (empty or containing \0) commit messages and appends a newline character to
+ * the commit message.
*
* @throws BadRequestException if the commit message is null or empty
* @returns the trimmed message with a trailing newline character
*/
- public static String checkAndSanitizeCommitMessage(String commitMessage)
+ public static String checkAndSanitizeCommitMessage(@Nullable String commitMessage)
throws BadRequestException {
- String wellFormedMessage = Strings.nullToEmpty(commitMessage).trim();
- if (wellFormedMessage.isEmpty()) {
+ String trimmed = Strings.nullToEmpty(commitMessage).trim();
+ if (trimmed.isEmpty()) {
throw new BadRequestException("Commit message cannot be null or empty");
}
- wellFormedMessage = wellFormedMessage + "\n";
- return wellFormedMessage;
+ if (trimmed.indexOf(0) >= 0) {
+ throw new BadRequestException("Commit message cannot have NUL character");
+ }
+ trimmed = trimmed + "\n";
+ return trimmed;
}
}
diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
index d90c4bd..c38f686 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -277,7 +277,7 @@
requestScopeOperations.setApiUser(user.getId());
exception.expect(AuthException.class);
- exception.expectMessage("not allowed to toggle work in progress");
+ exception.expectMessage("toggle work in progress state not permitted");
gApi.changes().id(changeId).setWorkInProgress();
}
@@ -323,7 +323,7 @@
requestScopeOperations.setApiUser(user.getId());
exception.expect(AuthException.class);
- exception.expectMessage("not allowed to toggle work in progress");
+ exception.expectMessage("toggle work in progress state not permitted");
gApi.changes().id(changeId).setReadyForReview();
}
@@ -500,6 +500,37 @@
}
@Test
+ public void toggleWorkInProgressStateByNonOwnerWithPermission() throws Exception {
+ PushOneCommit.Result r = createChange();
+ String changeId = r.getChangeId();
+ String refactor = "Needs some refactoring";
+ String ptal = "PTAL";
+
+ grant(
+ project,
+ "refs/heads/master",
+ Permission.TOGGLE_WORK_IN_PROGRESS_STATE,
+ false,
+ REGISTERED_USERS);
+
+ requestScopeOperations.setApiUser(user.getId());
+ gApi.changes().id(changeId).setWorkInProgress(refactor);
+
+ ChangeInfo info = gApi.changes().id(changeId).get();
+
+ assertThat(info.workInProgress).isTrue();
+ assertThat(Iterables.getLast(info.messages).message).contains(refactor);
+ assertThat(Iterables.getLast(info.messages).tag).contains(ChangeMessagesUtil.TAG_SET_WIP);
+
+ gApi.changes().id(changeId).setReadyForReview(ptal);
+
+ info = gApi.changes().id(changeId).get();
+ assertThat(info.workInProgress).isNull();
+ assertThat(Iterables.getLast(info.messages).message).contains(ptal);
+ assertThat(Iterables.getLast(info.messages).tag).contains(ChangeMessagesUtil.TAG_SET_READY);
+ }
+
+ @Test
public void reviewAndStartReview() throws Exception {
PushOneCommit.Result r = createWorkInProgressChange();
r.assertOkStatus();
@@ -588,17 +619,33 @@
ReviewInput in = ReviewInput.noScore().setWorkInProgress(true);
requestScopeOperations.setApiUser(user.getId());
exception.expect(AuthException.class);
- exception.expectMessage("not allowed to toggle work in progress");
+ exception.expectMessage("toggle work in progress state not permitted");
gApi.changes().id(r.getChangeId()).current().review(in);
}
@Test
+ public void reviewWithWorkInProgressByNonOwnerWithPermission() throws Exception {
+ PushOneCommit.Result r = createChange();
+ ReviewInput in = ReviewInput.noScore().setWorkInProgress(true);
+ grant(
+ project,
+ "refs/heads/master",
+ Permission.TOGGLE_WORK_IN_PROGRESS_STATE,
+ false,
+ REGISTERED_USERS);
+ requestScopeOperations.setApiUser(user.getId());
+ gApi.changes().id(r.getChangeId()).current().review(in);
+ ChangeInfo info = gApi.changes().id(r.getChangeId()).get();
+ assertThat(info.workInProgress).isTrue();
+ }
+
+ @Test
public void reviewWithReadyByNonOwnerReturnsError() throws Exception {
PushOneCommit.Result r = createChange();
ReviewInput in = ReviewInput.noScore().setReady(true);
requestScopeOperations.setApiUser(user.getId());
exception.expect(AuthException.class);
- exception.expectMessage("not allowed to toggle work in progress");
+ exception.expectMessage("toggle work in progress state not permitted");
gApi.changes().id(r.getChangeId()).current().review(in);
}
@@ -3533,6 +3580,18 @@
}
@Test
+ public void changeCommitMessageNullNotAllowed() throws Exception {
+ PushOneCommit.Result r = createChange();
+ assertThat(getCommitMessage(r.getChangeId()))
+ .isEqualTo("test commit\n\nChange-Id: " + r.getChangeId() + "\n");
+ exception.expect(BadRequestException.class);
+ exception.expectMessage("NUL character");
+ gApi.changes()
+ .id(r.getChangeId())
+ .setMessage("test\0commit\n\nChange-Id: " + r.getChangeId() + "\n");
+ }
+
+ @Test
public void changeCommitMessageWithWrongChangeIdFails() throws Exception {
PushOneCommit.Result otherChange = createChange();
PushOneCommit.Result r = createChange();
diff --git a/javatests/com/google/gerrit/acceptance/rest/binding/PluginProvidedChildRestApiBindingsIT.java b/javatests/com/google/gerrit/acceptance/rest/binding/PluginProvidedChildRestApiBindingsIT.java
new file mode 100644
index 0000000..27df565
--- /dev/null
+++ b/javatests/com/google/gerrit/acceptance/rest/binding/PluginProvidedChildRestApiBindingsIT.java
@@ -0,0 +1,160 @@
+// 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.rest.binding;
+
+import static com.google.gerrit.server.change.RevisionResource.REVISION_KIND;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
+import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.acceptance.rest.util.RestApiCallHelper;
+import com.google.gerrit.acceptance.rest.util.RestCall;
+import com.google.gerrit.extensions.registration.DynamicMap;
+import com.google.gerrit.extensions.restapi.ChildCollection;
+import com.google.gerrit.extensions.restapi.IdString;
+import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.extensions.restapi.RestApiModule;
+import com.google.gerrit.extensions.restapi.RestCollectionModifyView;
+import com.google.gerrit.extensions.restapi.RestModifyView;
+import com.google.gerrit.extensions.restapi.RestReadView;
+import com.google.gerrit.extensions.restapi.RestResource;
+import com.google.gerrit.extensions.restapi.RestView;
+import com.google.gerrit.reviewdb.client.PatchSet;
+import com.google.gerrit.server.change.RevisionResource;
+import com.google.inject.AbstractModule;
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+import com.google.inject.TypeLiteral;
+import org.junit.Test;
+
+/**
+ * Tests for checking plugin-provided REST API bindings nested under a core collection.
+ *
+ * <p>These tests only verify that the plugin-provided REST endpoints are correctly bound, they do
+ * not test the functionality of the plugin REST endpoints.
+ */
+public class PluginProvidedChildRestApiBindingsIT extends AbstractDaemonTest {
+
+ /** Resource to bind a child collection. */
+ public static final TypeLiteral<RestView<TestPluginResource>> TEST_KIND =
+ new TypeLiteral<RestView<TestPluginResource>>() {};
+
+ private static final String PLUGIN_NAME = "my-plugin";
+
+ private static final ImmutableSet<RestCall> TEST_CALLS =
+ ImmutableSet.of(
+ // Calls that have the plugin name as part of the collection name
+ RestCall.get("/changes/%s/revisions/%s/" + PLUGIN_NAME + "~test-collection/"),
+ RestCall.get("/changes/%s/revisions/%s/" + PLUGIN_NAME + "~test-collection/1/detail"),
+ RestCall.post("/changes/%s/revisions/%s/" + PLUGIN_NAME + "~test-collection/"),
+ RestCall.post("/changes/%s/revisions/%s/" + PLUGIN_NAME + "~test-collection/1/update"),
+ // Same tests but without the plugin name as part of the collection name. This works as
+ // long as there is no core collection with the same name (which takes precedence) and no
+ // other plugin binds a collection with the same name. We highly encourage plugin authors
+ // to use the fully qualified collection name instead.
+ RestCall.get("/changes/%s/revisions/%s/test-collection/"),
+ RestCall.get("/changes/%s/revisions/%s/test-collection/1/detail"),
+ RestCall.post("/changes/%s/revisions/%s/test-collection/"),
+ RestCall.post("/changes/%s/revisions/%s/test-collection/1/update"));
+
+ /**
+ * Module for all sys bindings.
+ *
+ * <p>TODO: This should actually just move into MyPluginHttpModule. However, that doesn't work
+ * currently. This TODO is for fixing this bug.
+ */
+ static class MyPluginSysModule extends AbstractModule {
+ @Override
+ public void configure() {
+ install(
+ new RestApiModule() {
+ @Override
+ public void configure() {
+ DynamicMap.mapOf(binder(), TEST_KIND);
+ child(REVISION_KIND, "test-collection").to(TestChildCollection.class);
+
+ postOnCollection(TEST_KIND).to(TestPostOnCollection.class);
+ post(TEST_KIND, "update").to(TestPost.class);
+ get(TEST_KIND, "detail").to(TestGet.class);
+ }
+ });
+ }
+ }
+
+ static class TestPluginResource implements RestResource {}
+
+ @Singleton
+ static class TestChildCollection
+ implements ChildCollection<RevisionResource, TestPluginResource> {
+ private final DynamicMap<RestView<TestPluginResource>> views;
+
+ @Inject
+ TestChildCollection(DynamicMap<RestView<TestPluginResource>> views) {
+ this.views = views;
+ }
+
+ @Override
+ public RestView<RevisionResource> list() throws RestApiException {
+ return (RestReadView<RevisionResource>) resource -> ImmutableList.of("one", "two");
+ }
+
+ @Override
+ public TestPluginResource parse(RevisionResource parent, IdString id) throws Exception {
+ return new TestPluginResource();
+ }
+
+ @Override
+ public DynamicMap<RestView<TestPluginResource>> views() {
+ return views;
+ }
+ }
+
+ @Singleton
+ static class TestPostOnCollection
+ implements RestCollectionModifyView<RevisionResource, TestPluginResource, String> {
+ @Override
+ public Object apply(RevisionResource parentResource, String input) throws Exception {
+ return "test";
+ }
+ }
+
+ @Singleton
+ static class TestPost implements RestModifyView<TestPluginResource, String> {
+ @Override
+ public String apply(TestPluginResource resource, String input) throws Exception {
+ return "test";
+ }
+ }
+
+ @Singleton
+ static class TestGet implements RestReadView<TestPluginResource> {
+ @Override
+ public String apply(TestPluginResource resource) throws Exception {
+ return "test";
+ }
+ }
+
+ @Test
+ public void testEndpoints() throws Exception {
+ PatchSet.Id patchSetId = createChange().getPatchSetId();
+ try (AutoCloseable ignored = installPlugin(PLUGIN_NAME, MyPluginSysModule.class, null, null)) {
+ RestApiCallHelper.execute(
+ adminRestSession,
+ TEST_CALLS.asList(),
+ String.valueOf(patchSetId.changeId.id),
+ String.valueOf(patchSetId.patchSetId));
+ }
+ }
+}
diff --git a/javatests/com/google/gerrit/acceptance/rest/binding/PluginProvidedRestApiBindingsIT.java b/javatests/com/google/gerrit/acceptance/rest/binding/PluginProvidedRestApiBindingsIT.java
deleted file mode 100644
index 82c065a..0000000
--- a/javatests/com/google/gerrit/acceptance/rest/binding/PluginProvidedRestApiBindingsIT.java
+++ /dev/null
@@ -1,75 +0,0 @@
-// 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.rest.binding;
-
-import static javax.servlet.http.HttpServletResponse.SC_OK;
-
-import com.google.common.collect.ImmutableList;
-import com.google.gerrit.acceptance.AbstractDaemonTest;
-import com.google.gerrit.acceptance.rest.util.RestApiCallHelper;
-import com.google.gerrit.acceptance.rest.util.RestCall;
-import com.google.inject.Singleton;
-import com.google.inject.servlet.ServletModule;
-import java.io.IOException;
-import javax.servlet.http.HttpServlet;
-import javax.servlet.http.HttpServletRequest;
-import javax.servlet.http.HttpServletResponse;
-import org.junit.Test;
-
-/**
- * Tests for checking plugin-provided REST API bindings.
- *
- * <p>These tests only verify that the plugin-provided REST endpoints are correctly bound, they do
- * not test the functionality of the plugin REST endpoints.
- */
-public class PluginProvidedRestApiBindingsIT extends AbstractDaemonTest {
-
- /**
- * Plugin REST endpoints bound by {@link MyPluginModule} with Guice serlvet definitions.
- *
- * <p>Each URL contains a placeholder for the plugin identifier.
- *
- * <p>Currently does not include any resource or documentation URLs, since those would require
- * installing a plugin from a jar, which is trickier than just defining a module in this file.
- */
- private static final ImmutableList<RestCall> SERVER_TOP_LEVEL_PLUGIN_ENDPOINTS =
- ImmutableList.of(RestCall.get("/plugins/%s/hello"));
-
- static class MyPluginModule extends ServletModule {
- @Override
- public void configureServlets() {
- serve("/hello").with(HelloServlet.class);
- }
- }
-
- @Singleton
- static class HelloServlet extends HttpServlet {
- private static final long serialVersionUID = 1L;
-
- @Override
- protected void doGet(HttpServletRequest req, HttpServletResponse res) throws IOException {
- res.setStatus(SC_OK);
- res.getWriter().println("Hello world");
- }
- }
-
- @Test
- public void serverPluginTopLevelEndpoints() throws Exception {
- String pluginName = "my-plugin";
- try (AutoCloseable ignored = installPlugin(pluginName, null, MyPluginModule.class, null)) {
- RestApiCallHelper.execute(adminRestSession, SERVER_TOP_LEVEL_PLUGIN_ENDPOINTS, pluginName);
- }
- }
-}
diff --git a/javatests/com/google/gerrit/acceptance/rest/binding/PluginProvidedRootRestApiBindingsIT.java b/javatests/com/google/gerrit/acceptance/rest/binding/PluginProvidedRootRestApiBindingsIT.java
new file mode 100644
index 0000000..8a8ea9b
--- /dev/null
+++ b/javatests/com/google/gerrit/acceptance/rest/binding/PluginProvidedRootRestApiBindingsIT.java
@@ -0,0 +1,199 @@
+// 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.rest.binding;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
+import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.acceptance.rest.util.RestApiCallHelper;
+import com.google.gerrit.acceptance.rest.util.RestCall;
+import com.google.gerrit.extensions.registration.DynamicMap;
+import com.google.gerrit.extensions.restapi.ChildCollection;
+import com.google.gerrit.extensions.restapi.IdString;
+import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.extensions.restapi.RestApiModule;
+import com.google.gerrit.extensions.restapi.RestCollectionModifyView;
+import com.google.gerrit.extensions.restapi.RestModifyView;
+import com.google.gerrit.extensions.restapi.RestReadView;
+import com.google.gerrit.extensions.restapi.RestResource;
+import com.google.gerrit.extensions.restapi.RestView;
+import com.google.gerrit.extensions.restapi.TopLevelResource;
+import com.google.gerrit.httpd.restapi.RestApiServlet;
+import com.google.inject.Inject;
+import com.google.inject.Provider;
+import com.google.inject.Singleton;
+import com.google.inject.TypeLiteral;
+import com.google.inject.servlet.ServletModule;
+import java.io.IOException;
+import javax.servlet.ServletException;
+import javax.servlet.ServletRequest;
+import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletRequestWrapper;
+import javax.servlet.http.HttpServletResponse;
+import org.junit.Test;
+
+/**
+ * Tests for checking plugin-provided REST API bindings directly under {@code /}.
+ *
+ * <p>These tests only verify that the plugin-provided REST endpoints are correctly bound, they do
+ * not test the functionality of the plugin REST endpoints.
+ */
+public class PluginProvidedRootRestApiBindingsIT extends AbstractDaemonTest {
+
+ /** Resource to bind a child collection. */
+ public static final TypeLiteral<RestView<TestPluginResource>> TEST_KIND =
+ new TypeLiteral<RestView<TestPluginResource>>() {};
+
+ private static final String PLUGIN_NAME = "my-plugin";
+
+ private static final ImmutableSet<RestCall> TEST_CALLS =
+ ImmutableSet.of(
+ RestCall.get("/plugins/" + PLUGIN_NAME + "/test-collection/"),
+ RestCall.get("/plugins/" + PLUGIN_NAME + "/test-collection/1/detail"),
+ RestCall.post("/plugins/" + PLUGIN_NAME + "/test-collection/"),
+ RestCall.post("/plugins/" + PLUGIN_NAME + "/test-collection/1/update"));
+
+ /** Module for all HTTP bindings. */
+ static class MyPluginHttpModule extends ServletModule {
+ @Override
+ public void configureServlets() {
+ bind(TestRootCollection.class);
+
+ install(
+ new RestApiModule() {
+ @Override
+ public void configure() {
+ DynamicMap.mapOf(binder(), TEST_KIND);
+
+ postOnCollection(TEST_KIND).to(TestPostOnCollection.class);
+ post(TEST_KIND, "update").to(TestPost.class);
+ get(TEST_KIND, "detail").to(TestGet.class);
+ }
+ });
+
+ serveRegex("/(?:a/)?test-collection/(.*)$").with(TestRestApiServlet.class);
+ }
+ }
+
+ @Singleton
+ static class TestRestApiServlet extends RestApiServlet {
+ private static final long serialVersionUID = 1L;
+
+ @Inject
+ TestRestApiServlet(RestApiServlet.Globals globals, Provider<TestRootCollection> collection) {
+ super(globals, collection);
+ }
+
+ @Override
+ public void service(ServletRequest servletRequest, ServletResponse servletResponse)
+ throws ServletException, IOException {
+ // This is...unfortunate. HttpPluginServlet (and/or ContextMapper) doesn't properly set the
+ // servlet path on the wrapped request. Based on what RestApiServlet produces for non-plugin
+ // requests, it should be:
+ // contextPath = "/plugins/checks"
+ // servletPath = "/checkers/"
+ // pathInfo = checkerUuid
+ // Instead it does:
+ // contextPath = "/plugins/checks"
+ // servletPath = ""
+ // pathInfo = "/checkers/" + checkerUuid
+ // This results in RestApiServlet splitting the pathInfo into ["", "checkers", checkerUuid],
+ // and it passes the "" to CheckersCollection#parse, which understandably, but unfortunately,
+ // fails.
+ //
+ // This frankly seems like a bug that should be fixed, but it would quite likely break
+ // existing plugins in confusing ways. So, we work around it by introducing our own request
+ // wrapper with the correct paths.
+ HttpServletRequest req = (HttpServletRequest) servletRequest;
+ String pathInfo = req.getPathInfo();
+ String correctServletPath = "/test-collection/";
+ String fixedPathInfo = pathInfo.substring(correctServletPath.length());
+ HttpServletRequestWrapper wrapped =
+ new HttpServletRequestWrapper(req) {
+ @Override
+ public String getServletPath() {
+ return correctServletPath;
+ }
+
+ @Override
+ public String getPathInfo() {
+ return fixedPathInfo;
+ }
+ };
+
+ super.service(wrapped, (HttpServletResponse) servletResponse);
+ }
+ }
+
+ static class TestPluginResource implements RestResource {}
+
+ @Singleton
+ static class TestRootCollection implements ChildCollection<TopLevelResource, TestPluginResource> {
+ private final DynamicMap<RestView<TestPluginResource>> views;
+
+ @Inject
+ TestRootCollection(DynamicMap<RestView<TestPluginResource>> views) {
+ this.views = views;
+ }
+
+ @Override
+ public RestView<TopLevelResource> list() throws RestApiException {
+ return (RestReadView<TopLevelResource>) resource -> ImmutableList.of("one", "two");
+ }
+
+ @Override
+ public TestPluginResource parse(TopLevelResource parent, IdString id) throws Exception {
+ return new TestPluginResource();
+ }
+
+ @Override
+ public DynamicMap<RestView<TestPluginResource>> views() {
+ return views;
+ }
+ }
+
+ @Singleton
+ static class TestPostOnCollection
+ implements RestCollectionModifyView<TopLevelResource, TestPluginResource, String> {
+ @Override
+ public Object apply(TopLevelResource parentResource, String input) throws Exception {
+ return "test";
+ }
+ }
+
+ @Singleton
+ static class TestPost implements RestModifyView<TestPluginResource, String> {
+ @Override
+ public String apply(TestPluginResource resource, String input) throws Exception {
+ return "test";
+ }
+ }
+
+ @Singleton
+ static class TestGet implements RestReadView<TestPluginResource> {
+ @Override
+ public String apply(TestPluginResource resource) throws Exception {
+ return "test";
+ }
+ }
+
+ @Test
+ public void testEndpoints() throws Exception {
+ try (AutoCloseable ignored = installPlugin(PLUGIN_NAME, null, MyPluginHttpModule.class, null)) {
+ RestApiCallHelper.execute(adminRestSession, TEST_CALLS.asList());
+ }
+ }
+}
diff --git a/javatests/com/google/gerrit/acceptance/rest/project/PluginAccessIT.java b/javatests/com/google/gerrit/acceptance/rest/project/PluginAccessIT.java
index 1e6afa8..e7663f7 100644
--- a/javatests/com/google/gerrit/acceptance/rest/project/PluginAccessIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/project/PluginAccessIT.java
@@ -26,15 +26,21 @@
import com.google.gerrit.extensions.api.access.ProjectAccessInfo;
import com.google.gerrit.extensions.api.access.ProjectAccessInput;
import com.google.gerrit.extensions.config.CapabilityDefinition;
+import com.google.gerrit.extensions.config.PluginProjectPermissionDefinition;
import com.google.gerrit.server.group.SystemGroupBackend;
+import com.google.gerrit.server.permissions.PluginPermissionsUtil;
import com.google.inject.AbstractModule;
+import com.google.inject.Inject;
import com.google.inject.Module;
+import java.util.Set;
import org.junit.Test;
-public class PluginAccessIT extends AbstractDaemonTest {
+public final class PluginAccessIT extends AbstractDaemonTest {
+ private static final String TEST_PLUGIN_NAME = "gerrit";
+ private static final String TEST_PLUGIN_CAPABILITY = "aPluginCapability";
+ private static final String TEST_PLUGIN_PROJECT_PERMISSION = "aPluginProjectPermission";
- private static final String CORE_PLUGIN_PREFIX = "gerrit-";
- private static final String PLUGIN_CAPABILITY = "printHello";
+ @Inject PluginPermissionsUtil pluginPermissionsUtil;
@Override
public Module createModule() {
@@ -42,12 +48,21 @@
@Override
protected void configure() {
bind(CapabilityDefinition.class)
- .annotatedWith(Exports.named(PLUGIN_CAPABILITY))
+ .annotatedWith(Exports.named(TEST_PLUGIN_CAPABILITY))
.toInstance(
new CapabilityDefinition() {
@Override
public String getDescription() {
- return "Print Hello";
+ return "A Plugin Capability";
+ }
+ });
+ bind(PluginProjectPermissionDefinition.class)
+ .annotatedWith(Exports.named(TEST_PLUGIN_PROJECT_PERMISSION))
+ .toInstance(
+ new PluginProjectPermissionDefinition() {
+ @Override
+ public String getDescription() {
+ return "A Plugin Project Permission";
}
});
}
@@ -55,24 +70,47 @@
}
@Test
- public void addPluginCapability() throws Exception {
- ProjectAccessInput accessInput = new ProjectAccessInput();
- AccessSectionInfo accessSectionInfo = new AccessSectionInfo();
- PermissionInfo email = new PermissionInfo(null, null);
- PermissionRuleInfo pri = new PermissionRuleInfo(PermissionRuleInfo.Action.ALLOW, false);
+ public void setAccessAddPluginCapabilitySucceed() throws Exception {
+ String pluginCapability = TEST_PLUGIN_NAME + "-" + TEST_PLUGIN_CAPABILITY;
+ ProjectAccessInput accessInput =
+ createAccessInput(AccessSection.GLOBAL_CAPABILITIES, pluginCapability);
- email.rules = ImmutableMap.of(SystemGroupBackend.REGISTERED_USERS.get(), pri);
- accessSectionInfo.permissions = ImmutableMap.of(CORE_PLUGIN_PREFIX + PLUGIN_CAPABILITY, email);
- accessInput.add = ImmutableMap.of(AccessSection.GLOBAL_CAPABILITIES, accessSectionInfo);
-
- ProjectAccessInfo updatedAccessSectionInfo =
+ ProjectAccessInfo projectAccessInfo =
gApi.projects().name(allProjects.get()).access(accessInput);
- assertThat(
- updatedAccessSectionInfo
- .local
- .get(AccessSection.GLOBAL_CAPABILITIES)
- .permissions
- .keySet())
- .containsAllIn(accessSectionInfo.permissions.keySet());
+
+ Set<String> capabilities =
+ projectAccessInfo.local.get(AccessSection.GLOBAL_CAPABILITIES).permissions.keySet();
+ assertThat(capabilities).contains(pluginCapability);
+ // Verifies the plugin defined capability could be listed.
+ assertThat(pluginPermissionsUtil.collectPluginCapabilities()).containsKey(pluginCapability);
+ }
+
+ @Test
+ public void setAccessAddPluginProjectPermissionSucceed() throws Exception {
+ String pluginProjectPermission =
+ "plugin-" + TEST_PLUGIN_NAME + "-" + TEST_PLUGIN_PROJECT_PERMISSION;
+ String accessSection = "refs/heads/plugin-permission";
+ ProjectAccessInput accessInput = createAccessInput(accessSection, pluginProjectPermission);
+
+ ProjectAccessInfo projectAccessInfo =
+ gApi.projects().name(allProjects.get()).access(accessInput);
+
+ Set<String> permissions = projectAccessInfo.local.get(accessSection).permissions.keySet();
+ assertThat(permissions).contains(pluginProjectPermission);
+ // Verifies the plugin defined capability could be listed.
+ assertThat(pluginPermissionsUtil.collectPluginProjectPermissions())
+ .containsKey(pluginProjectPermission);
+ }
+
+ private static ProjectAccessInput createAccessInput(String accessSection, String permissionName) {
+ ProjectAccessInput accessInput = new ProjectAccessInput();
+ PermissionRuleInfo ruleInfo = new PermissionRuleInfo(PermissionRuleInfo.Action.ALLOW, false);
+ PermissionInfo email = new PermissionInfo(null, null);
+ email.rules = ImmutableMap.of(SystemGroupBackend.REGISTERED_USERS.get(), ruleInfo);
+ AccessSectionInfo accessSectionInfo = new AccessSectionInfo();
+ accessSectionInfo.permissions = ImmutableMap.of(permissionName, email);
+ accessInput.add = ImmutableMap.of(accessSection, accessSectionInfo);
+
+ return accessInput;
}
}
diff --git a/javatests/com/google/gerrit/server/config/ListCapabilitiesTest.java b/javatests/com/google/gerrit/server/config/ListCapabilitiesTest.java
index 081a2f7..30fabdc 100644
--- a/javatests/com/google/gerrit/server/config/ListCapabilitiesTest.java
+++ b/javatests/com/google/gerrit/server/config/ListCapabilitiesTest.java
@@ -16,9 +16,11 @@
import static com.google.common.truth.Truth.assertThat;
+import com.google.common.collect.ImmutableSet;
import com.google.gerrit.common.data.GlobalCapability;
import com.google.gerrit.extensions.annotations.Exports;
import com.google.gerrit.extensions.config.CapabilityDefinition;
+import com.google.gerrit.extensions.config.PluginProjectPermissionDefinition;
import com.google.gerrit.extensions.registration.DynamicMap;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.server.CurrentUser;
@@ -44,8 +46,18 @@
@Override
protected void configure() {
DynamicMap.mapOf(binder(), CapabilityDefinition.class);
+ DynamicMap.mapOf(binder(), PluginProjectPermissionDefinition.class);
bind(CapabilityDefinition.class)
- .annotatedWith(Exports.named("printHello"))
+ .annotatedWith(Exports.named("foo"))
+ .toInstance(
+ new CapabilityDefinition() {
+ @Override
+ public String getDescription() {
+ return "Print Hello";
+ }
+ });
+ bind(CapabilityDefinition.class)
+ .annotatedWith(Exports.named("bar"))
.toInstance(
new CapabilityDefinition() {
@Override
@@ -69,10 +81,11 @@
assertThat(m.get(id).name).isNotNull();
}
- String pluginCapability = "gerrit-printHello";
- assertThat(m).containsKey(pluginCapability);
- assertThat(m.get(pluginCapability).id).isEqualTo(pluginCapability);
- assertThat(m.get(pluginCapability).name).isEqualTo("Print Hello");
+ for (String pluginCapability : ImmutableSet.of("gerrit-foo", "gerrit-bar")) {
+ assertThat(m).containsKey(pluginCapability);
+ assertThat(m.get(pluginCapability).id).isEqualTo(pluginCapability);
+ assertThat(m.get(pluginCapability).name).isEqualTo("Print Hello");
+ }
}
@Singleton
diff --git a/javatests/com/google/gerrit/server/permissions/PluginPermissionsUtilTest.java b/javatests/com/google/gerrit/server/permissions/PluginPermissionsUtilTest.java
new file mode 100644
index 0000000..85d85fb
--- /dev/null
+++ b/javatests/com/google/gerrit/server/permissions/PluginPermissionsUtilTest.java
@@ -0,0 +1,56 @@
+// 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.server.permissions;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.server.permissions.PluginPermissionsUtil.isPluginPermission;
+
+import com.google.common.collect.ImmutableList;
+import org.junit.Test;
+
+/** Small tests for {@link PluginPermissionsUtil}. */
+public final class PluginPermissionsUtilTest {
+
+ @Test
+ public void isPluginPermissionReturnsTrueForValidName() {
+ // "-" is allowed for a plugin name. Here "foo-a" should be the name of the plugin.
+ ImmutableList<String> validPluginPermissions =
+ ImmutableList.of("plugin-foo-a", "plugin-foo-a-b");
+
+ for (String permission : validPluginPermissions) {
+ assertThat(isPluginPermission(permission))
+ .named("valid plugin permission: %s", permission)
+ .isTrue();
+ }
+ }
+
+ @Test
+ public void isPluginPermissionReturnsFalseForInvalidName() {
+ ImmutableList<String> inValidPluginPermissions =
+ ImmutableList.of(
+ "create",
+ "label-Code-Review",
+ "plugin-foo",
+ "plugin-foo",
+ "plugin-foo-a-",
+ "plugin-foo-a1");
+
+ for (String permission : inValidPluginPermissions) {
+ assertThat(isPluginPermission(permission))
+ .named("invalid plugin permission: %s", permission)
+ .isFalse();
+ }
+ }
+}
diff --git a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
index 5eecd0f..100c25e 100644
--- a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
+++ b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
@@ -14,6 +14,7 @@
package com.google.gerrit.server.query.change;
+import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.TruthJUnit.assume;
import static com.google.gerrit.extensions.client.ListChangesOption.DETAILED_LABELS;
@@ -69,6 +70,7 @@
import com.google.gerrit.index.FieldDef;
import com.google.gerrit.index.IndexConfig;
import com.google.gerrit.index.Schema;
+import com.google.gerrit.index.query.Predicate;
import com.google.gerrit.lifecycle.LifecycleManager;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Branch;
@@ -3134,6 +3136,26 @@
assertQuery("assignee:self", change);
}
+ @Test
+ public void none() throws Exception {
+ TestRepository<Repo> repo = createProject("repo");
+ Change change = insert(repo, newChange(repo));
+
+ assertQuery(ChangeIndexPredicate.none());
+
+ for (Predicate<ChangeData> matchingOneChange :
+ ImmutableList.of(
+ // One index query, one post-filtering query.
+ queryBuilder.parse(change.getId().toString()),
+ queryBuilder.parse("ownerin:Administrators"))) {
+ assertQuery(matchingOneChange, change);
+ assertQuery(Predicate.or(ChangeIndexPredicate.none(), matchingOneChange), change);
+ assertQuery(Predicate.and(ChangeIndexPredicate.none(), matchingOneChange));
+ assertQuery(
+ Predicate.and(Predicate.not(ChangeIndexPredicate.none()), matchingOneChange), change);
+ }
+ }
+
protected ChangeInserter newChange(TestRepository<Repo> repo) throws Exception {
return newChange(repo, null, null, null, null, false);
}
@@ -3307,21 +3329,35 @@
List<ChangeInfo> result = query.get();
Iterable<Change.Id> ids = ids(result);
assertThat(ids)
- .named(format(query, ids, changes))
+ .named(format(query.getQuery(), ids, changes))
.containsExactlyElementsIn(Arrays.asList(changes))
.inOrder();
return result;
}
- private String format(
- QueryRequest query, Iterable<Change.Id> actualIds, Change.Id... expectedChanges)
+ protected void assertQuery(Predicate<ChangeData> predicate, Change... changes) throws Exception {
+ ImmutableList<Change.Id> actualIds =
+ queryProvider
+ .get()
+ .query(predicate)
+ .stream()
+ .map(ChangeData::getId)
+ .collect(toImmutableList());
+ Change.Id[] expectedIds = Arrays.stream(changes).map(Change::getId).toArray(Change.Id[]::new);
+ assertThat(actualIds)
+ .named(format(predicate.toString(), actualIds, expectedIds))
+ .containsExactlyElementsIn(expectedIds)
+ .inOrder();
+ }
+
+ private String format(String query, Iterable<Change.Id> actualIds, Change.Id... expectedChanges)
throws RestApiException {
- StringBuilder b = new StringBuilder();
- b.append("query '").append(query.getQuery()).append("' with expected changes ");
- b.append(format(Arrays.asList(expectedChanges)));
- b.append(" and result ");
- b.append(format(actualIds));
- return b.toString();
+ return "query '"
+ + query
+ + "' with expected changes "
+ + format(Arrays.asList(expectedChanges))
+ + " and result "
+ + format(actualIds);
}
private String format(Iterable<Change.Id> changeIds) throws RestApiException {
diff --git a/plugins/codemirror-editor b/plugins/codemirror-editor
index c4cf42b..0ffcc1c 160000
--- a/plugins/codemirror-editor
+++ b/plugins/codemirror-editor
@@ -1 +1 @@
-Subproject commit c4cf42b96a049a0fb854bcbcb85b56a82d91a009
+Subproject commit 0ffcc1c5865bbfecb811d8631e010251fae04317
diff --git a/polygerrit-ui/app/behaviors/gr-access-behavior/gr-access-behavior.html b/polygerrit-ui/app/behaviors/gr-access-behavior/gr-access-behavior.html
index fb0c685..e949a87 100644
--- a/polygerrit-ui/app/behaviors/gr-access-behavior/gr-access-behavior.html
+++ b/polygerrit-ui/app/behaviors/gr-access-behavior/gr-access-behavior.html
@@ -120,6 +120,10 @@
id: 'submitAs',
name: 'Submit (On Behalf Of)',
},
+ toggleWipState: {
+ id: 'toggleWipState',
+ name: 'Toggle Work In Progress State',
+ },
viewDrafts: {
id: 'viewDrafts',
name: 'View Drafts',
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html
index fc0cab0..730f527 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html
@@ -519,8 +519,27 @@
</div>
</div>
</section>
+
<section class="patchInfo">
- <gr-file-list-header
+ <template is="dom-if" if="[[_showPrimaryTabs]]">
+ <paper-tabs id="primaryTabs" on-selected-changed="_handleFileTabChange">
+ <paper-tab>Files</paper-tab>
+ <template is="dom-repeat" items="[[_dynamicTabHeaderEndpoints]]"
+ as="tabHeader">
+ <paper-tab>
+ <gr-endpoint-decorator name$="[[tabHeader]]">
+ <gr-endpoint-param name="change" value="[[_change]]">
+ </gr-endpoint-param>
+ <gr-endpoint-param name="revision" value="[[_selectedRevision]]">
+ </gr-endpoint-param>
+ </gr-endpoint-decorator>
+ </paper-tab>
+ </template>
+ </paper-tabs>
+ </template>
+
+ <div hidden$="[[!_showFileTabContent]]">
+ <gr-file-list-header
id="fileListHeader"
account="[[_account]]"
all-patch-sets="[[_allPatchSets]]"
@@ -539,6 +558,7 @@
base-patch-num="{{_patchRange.basePatchNum}}"
files-expanded="[[_filesExpanded]]"
diff-prefs-disabled="[[_diffPrefsDisabled]]"
+ show-title="[[!_showPrimaryTabs]]"
on-open-diff-prefs="_handleOpenDiffPrefs"
on-open-download-dialog="_handleOpenDownloadDialog"
on-open-upload-help-dialog="_handleOpenUploadHelpDialog"
@@ -566,7 +586,18 @@
on-files-shown-changed="_setShownFiles"
on-file-action-tap="_handleFileActionTap"
on-reload-drafts="_reloadDraftsWithCallback"></gr-file-list>
+ </div>
+
+ <template is="dom-if" if="[[!_showFileTabContent]]">
+ <gr-endpoint-decorator name$="[[_selectedFilesTabPluginEndpoint]]">
+ <gr-endpoint-param name="change" value="[[_change]]">
+ </gr-endpoint-param>
+ <gr-endpoint-param name="revision" value="[[_selectedRevision]]">
+ </gr-endpoint-param>
+ </gr-endpoint-decorator>
+ </template>
</section>
+
<gr-endpoint-decorator name="change-view-integration">
<gr-endpoint-param name="change" value="[[_change]]">
</gr-endpoint-param>
@@ -575,7 +606,7 @@
</gr-endpoint-decorator>
<paper-tabs
id="commentTabs"
- on-selected-changed="_handleTabChange">
+ on-selected-changed="_handleCommentTabChange">
<paper-tab class="changeLog">Change Log</paper-tab>
<paper-tab
class="commentThreads">
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
index bb51fcc..defbcdc 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
@@ -253,6 +253,25 @@
type: Boolean,
value: true,
},
+ _showFileTabContent: {
+ type: Boolean,
+ value: true,
+ },
+ /** @type {Array<string>} */
+ _dynamicTabHeaderEndpoints: {
+ type: Array,
+ },
+ _showPrimaryTabs: {
+ type: Boolean,
+ computed: '_computeShowPrimaryTabs(_dynamicTabHeaderEndpoints)',
+ },
+ /** @type {Array<string>} */
+ _dynamicTabContentEndpoints: {
+ type: Array,
+ },
+ _selectedFilesTabPluginEndpoint: {
+ type: String,
+ },
},
behaviors: [
@@ -310,6 +329,17 @@
this._setDiffViewMode();
});
+ Gerrit.awaitPluginsLoaded().then(() => {
+ this._dynamicTabHeaderEndpoints =
+ Gerrit._endpoints.getDynamicEndpoints('change-view-tab-header');
+ this._dynamicTabContentEndpoints =
+ Gerrit._endpoints.getDynamicEndpoints('change-view-tab-content');
+ if (this._dynamicTabContentEndpoints.length
+ !== this._dynamicTabHeaderEndpoints.length) {
+ console.warn('Different number of tab headers and tab content.');
+ }
+ });
+
this.addEventListener('comment-save', this._handleCommentSave.bind(this));
this.addEventListener('comment-refresh', this._reloadDrafts.bind(this));
this.addEventListener('comment-discard',
@@ -368,10 +398,18 @@
}
},
- _handleTabChange() {
+ _handleCommentTabChange() {
this._showMessagesView = this.$.commentTabs.selected === 0;
},
+ _handleFileTabChange() {
+ const selectedIndex = this.$$('#primaryTabs').selected;
+ this._showFileTabContent = selectedIndex === 0;
+ // Initial tab is the static files list.
+ this._selectedFilesTabPluginEndpoint =
+ this._dynamicTabContentEndpoints[selectedIndex - 1];
+ },
+
_handleEditCommitMessage(e) {
this._editingCommitMessage = true;
this.$.commitMessageEditor.focusTextarea();
@@ -699,6 +737,8 @@
// Selected has to be set after the paper-tabs are visible because
// the selected underline depends on calculations made by the browser.
this.$.commentTabs.selected = 0;
+ const primaryTabs = this.$$('#primaryTabs');
+ if (primaryTabs) primaryTabs.selected = 0;
this.async(() => {
if (this.viewState.scrollTop) {
@@ -821,6 +861,10 @@
this.fire('title-change', {title});
},
+ _computeShowPrimaryTabs(dynamicTabContentEndpoints) {
+ return dynamicTabContentEndpoints.length > 0;
+ },
+
_computeChangeUrl(change) {
return Gerrit.Nav.getUrlForChange(change);
},
diff --git a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.html b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.html
index 924ddab..1a307f7 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.html
+++ b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.html
@@ -154,7 +154,10 @@
</style>
<div class$="patchInfo-header [[_computeEditModeClass(editMode)]] [[_computePatchInfoClass(patchNum, allPatchSets)]]">
<div class="patchInfo-left">
- <h3 class="label">Files</h3>
+ <template is="dom-if"
+ if="[[showTitle]]">
+ <h3 class="label">Files</h3>
+ </template>
<div class="patchInfoContent">
<gr-patch-range-select
id="rangeSelect"
diff --git a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.js b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.js
index b9e6288..031f2d5 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.js
+++ b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.js
@@ -81,6 +81,10 @@
type: String,
value: '',
},
+ showTitle: {
+ type: Boolean,
+ value: true,
+ },
_descriptionReadOnly: {
type: Boolean,
computed: '_computeDescriptionReadOnly(loggedIn, change, account)',
diff --git a/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.js b/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.js
index 6699bd1..17f883f 100644
--- a/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.js
+++ b/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.js
@@ -65,6 +65,7 @@
'is:reviewed',
'is:reviewer',
'is:starred',
+ 'is:submittable',
'is:watched',
'is:wip',
'label:',
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html
index 2227b45..29cc3b9 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html
@@ -80,12 +80,6 @@
*/
/**
- * Fired when the diff is rendered.
- *
- * @event render
- */
-
- /**
* Fired when the diff finishes rendering text content, but not
* necessarily syntax highlights.
*
@@ -169,8 +163,6 @@
return this.$.syntaxLayer.process().then(() => {
reporting.timeEnd(TimingLabel.SYNTAX);
reporting.timeEnd(TimingLabel.TOTAL);
- this.dispatchEvent(
- new CustomEvent('render', {bubbles: true}));
});
});
},
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html
index 0fc4748..67d8c19 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html
@@ -850,14 +850,13 @@
assert.strictEqual(sections[1], section[1]);
});
- test('render-start and render are fired', done => {
+ test('render-start and render-content are fired', done => {
const dispatchEventStub = sandbox.stub(element, 'dispatchEvent');
element.render(keyLocations, {}).then(() => {
const firedEventTypes = dispatchEventStub.getCalls()
.map(c => { return c.args[0].type; });
assert.include(firedEventTypes, 'render-start');
assert.include(firedEventTypes, 'render-content');
- assert.include(firedEventTypes, 'render');
done();
});
});
@@ -869,10 +868,6 @@
test('rendering large diff disables syntax', done => {
// Before it renders, set the first diff line to 500 '*' characters.
element.diff.content[0].a = [new Array(501).join('*')];
- element.addEventListener('render', () => {
- assert.isFalse(element.$.syntaxLayer.enabled);
- done();
- });
const prefs = {
line_length: 10,
show_tabs: true,
@@ -880,7 +875,10 @@
context: -1,
syntax_highlighting: true,
};
- element.render(keyLocations, prefs);
+ element.render(keyLocations, prefs).then(() => {
+ assert.isFalse(element.$.syntaxLayer.enabled);
+ done();
+ });
});
test('cancel', () => {
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
index d5fb878..5ecc2fc3 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
@@ -116,11 +116,18 @@
* @event show-auth-required
*/
- /**
- * Fired when a comment is created
- *
- * @event create-comment
- */
+ /**
+ * Fired when a comment is created
+ *
+ * @event create-comment
+ */
+
+ /**
+ * Fired when rendering, including syntax highlighting, is done. Also fired
+ * when no rendering can be done because required preferences are not set.
+ *
+ * @event render
+ */
properties: {
changeNum: String,
@@ -700,7 +707,11 @@
this._showWarning = false;
const keyLocations = this._computeKeyLocations();
- this.$.diffBuilder.render(keyLocations, this._getBypassPrefs());
+ this.$.diffBuilder.render(keyLocations, this._getBypassPrefs())
+ .then(() => {
+ this.dispatchEvent(
+ new CustomEvent('render', {bubbles: true}));
+ });
},
_handleRenderContent() {
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html
index 300807c..849ad0d 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html
@@ -943,7 +943,8 @@
setup(() => {
element = fixture('basic');
element.prefs = {};
- renderStub = sandbox.stub(element.$.diffBuilder, 'render');
+ renderStub = sandbox.stub(element.$.diffBuilder, 'render')
+ .returns(new Promise(() => {}));
});
test('lineOfInterest is a key location', () => {
diff --git a/resources/com/google/gerrit/httpd/raw/PolyGerritIndexHtml.soy b/resources/com/google/gerrit/httpd/raw/PolyGerritIndexHtml.soy
index 9801b44..e4f699c 100644
--- a/resources/com/google/gerrit/httpd/raw/PolyGerritIndexHtml.soy
+++ b/resources/com/google/gerrit/httpd/raw/PolyGerritIndexHtml.soy
@@ -16,16 +16,14 @@
{namespace com.google.gerrit.httpd.raw}
-/**
- * @param canonicalPath
- * @param staticResourcePath
- * @param? assetsPath {string} URL to static assets root, if served from CDN.
- * @param? assetsBundle {string} Assets bundle .html file, served from $assetsPath.
- * @param? faviconPath
- * @param? versionInfo
- * @param? deprecateGwtUi
- */
{template .Index}
+ {@param canonicalPath: ?}
+ {@param staticResourcePath: ?}
+ {@param? assetsPath: ?} /** {string} URL to static assets root, if served from CDN. */
+ {@param? assetsBundle: ?} /** {string} Assets bundle .html file, served from $assetsPath. */
+ {@param? faviconPath: ?}
+ {@param? versionInfo: ?}
+ {@param? deprecateGwtUi: ?}
<!DOCTYPE html>{\n}
<html lang="en">{\n}
<meta charset="utf-8">{\n}