Merge changes I15829138,I689998da,I05c81db8
* changes:
SubmoduleOp: Make the constructor a pure one
SubscriptionGraph: Inject SubscriptionGraph.Factory
SubscriptionGraph: polish the class.
diff --git a/Documentation/rest-api-projects.txt b/Documentation/rest-api-projects.txt
index a4e27b3..723b45a 100644
--- a/Documentation/rest-api-projects.txt
+++ b/Documentation/rest-api-projects.txt
@@ -3586,7 +3586,7 @@
|`plugin_config` |optional|
Plugin configuration as map which maps the plugin name to a map of
parameter names to link:#config-parameter-info[ConfigParameterInfo]
-entities.
+entities. Only filled for users who have read access to `refs/meta/config`.
|`actions` |optional|
Actions the caller might be able to perform on this project. The
information is a map of view names to
diff --git a/java/com/google/gerrit/acceptance/ExtensionRegistry.java b/java/com/google/gerrit/acceptance/ExtensionRegistry.java
index cfe7964..a5d8d19 100644
--- a/java/com/google/gerrit/acceptance/ExtensionRegistry.java
+++ b/java/com/google/gerrit/acceptance/ExtensionRegistry.java
@@ -36,6 +36,7 @@
import com.google.gerrit.server.ExceptionHook;
import com.google.gerrit.server.account.GroupBackend;
import com.google.gerrit.server.change.ChangeETagComputation;
+import com.google.gerrit.server.config.ProjectConfigEntry;
import com.google.gerrit.server.git.ChangeMessageModifier;
import com.google.gerrit.server.git.validators.CommitValidationListener;
import com.google.gerrit.server.git.validators.OnSubmitValidationListener;
@@ -79,6 +80,7 @@
private final DynamicSet<WorkInProgressStateChangedListener> workInProgressStateChangedListeners;
private final DynamicMap<CapabilityDefinition> capabilityDefinitions;
private final DynamicMap<PluginProjectPermissionDefinition> pluginProjectPermissionDefinitions;
+ private final DynamicMap<ProjectConfigEntry> pluginConfigEntries;
@Inject
ExtensionRegistry(
@@ -107,7 +109,8 @@
DynamicSet<OnSubmitValidationListener> onSubmitValidationListeners,
DynamicSet<WorkInProgressStateChangedListener> workInProgressStateChangedListeners,
DynamicMap<CapabilityDefinition> capabilityDefinitions,
- DynamicMap<PluginProjectPermissionDefinition> pluginProjectPermissionDefinitions) {
+ DynamicMap<PluginProjectPermissionDefinition> pluginProjectPermissionDefinitions,
+ DynamicMap<ProjectConfigEntry> pluginConfigEntries) {
this.accountIndexedListeners = accountIndexedListeners;
this.changeIndexedListeners = changeIndexedListeners;
this.groupIndexedListeners = groupIndexedListeners;
@@ -134,6 +137,7 @@
this.workInProgressStateChangedListeners = workInProgressStateChangedListeners;
this.capabilityDefinitions = capabilityDefinitions;
this.pluginProjectPermissionDefinitions = pluginProjectPermissionDefinitions;
+ this.pluginConfigEntries = pluginConfigEntries;
}
public Registration newRegistration() {
@@ -254,6 +258,10 @@
return add(pluginProjectPermissionDefinitions, pluginProjectPermissionDefinition, exportName);
}
+ public Registration add(ProjectConfigEntry pluginConfigEntry, String exportName) {
+ return add(pluginConfigEntries, pluginConfigEntry, exportName);
+ }
+
private <T> Registration add(DynamicSet<T> dynamicSet, T extension) {
return add(dynamicSet, extension, "gerrit");
}
diff --git a/java/com/google/gerrit/server/notedb/ChangeUpdate.java b/java/com/google/gerrit/server/notedb/ChangeUpdate.java
index 348ab7d..1c81694 100644
--- a/java/com/google/gerrit/server/notedb/ChangeUpdate.java
+++ b/java/com/google/gerrit/server/notedb/ChangeUpdate.java
@@ -659,7 +659,9 @@
addFooter(msg, e.getValue().getFooterKey());
noteUtil.appendAccountIdIdentString(msg, e.getKey()).append('\n');
}
+
applyReviewerUpdatesToAttentionSet();
+
for (Map.Entry<Address, ReviewerStateInternal> e : reviewersByEmail.entrySet()) {
addFooter(msg, e.getValue().getByEmailFooterKey(), e.getKey().toString());
}
@@ -766,8 +768,10 @@
private void applyReviewerUpdatesToAttentionSet() {
if ((workInProgress != null && workInProgress == true)
- || getNotes().getChange().isWorkInProgress()) {
- // Users shouldn't be added to the attention set if the change is work in progress.
+ || getNotes().getChange().isWorkInProgress()
+ || status == Change.Status.MERGED) {
+ // Attention set shouldn't change here for changes that are work in progress or are about to
+ // be submitted.
return;
}
Set<Account.Id> currentReviewers =
diff --git a/java/com/google/gerrit/server/restapi/project/GetConfig.java b/java/com/google/gerrit/server/restapi/project/GetConfig.java
index ce45e7d..ad66587 100644
--- a/java/com/google/gerrit/server/restapi/project/GetConfig.java
+++ b/java/com/google/gerrit/server/restapi/project/GetConfig.java
@@ -24,6 +24,9 @@
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.permissions.PermissionBackend;
+import com.google.gerrit.server.permissions.PermissionBackendException;
+import com.google.gerrit.server.permissions.ProjectPermission;
import com.google.gerrit.server.project.ProjectResource;
import com.google.inject.Inject;
import com.google.inject.Singleton;
@@ -34,6 +37,7 @@
private final DynamicMap<ProjectConfigEntry> pluginConfigEntries;
private final PluginConfigFactory cfgFactory;
private final AllProjectsName allProjects;
+ private final PermissionBackend permissionBackend;
private final UiActions uiActions;
private final DynamicMap<RestView<ProjectResource>> views;
@@ -43,24 +47,31 @@
DynamicMap<ProjectConfigEntry> pluginConfigEntries,
PluginConfigFactory cfgFactory,
AllProjectsName allProjects,
+ PermissionBackend permissionBackend,
UiActions uiActions,
DynamicMap<RestView<ProjectResource>> views) {
this.serverEnableSignedPush = serverEnableSignedPush;
this.pluginConfigEntries = pluginConfigEntries;
this.allProjects = allProjects;
this.cfgFactory = cfgFactory;
+ this.permissionBackend = permissionBackend;
this.uiActions = uiActions;
this.views = views;
}
@Override
- public Response<ConfigInfo> apply(ProjectResource resource) {
+ public Response<ConfigInfo> apply(ProjectResource resource) throws PermissionBackendException {
+ boolean readConfigAllowed =
+ permissionBackend
+ .currentUser()
+ .project(resource.getNameKey())
+ .test(ProjectPermission.READ_CONFIG);
return Response.ok(
new ConfigInfoImpl(
serverEnableSignedPush,
resource.getProjectState(),
resource.getUser(),
- pluginConfigEntries,
+ readConfigAllowed ? pluginConfigEntries : DynamicMap.emptyMap(),
cfgFactory,
allProjects,
uiActions,
diff --git a/javatests/com/google/gerrit/acceptance/api/project/ProjectIT.java b/javatests/com/google/gerrit/acceptance/api/project/ProjectIT.java
index cb22849..840d3e0 100644
--- a/javatests/com/google/gerrit/acceptance/api/project/ProjectIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/project/ProjectIT.java
@@ -696,6 +696,31 @@
}
@Test
+ public void pluginConfigsReturnedWhenRefsMetaConfigReadable() throws Exception {
+ ProjectConfigEntry entry = new ProjectConfigEntry("enabled", "true");
+ try (Registration ignored =
+ extensionRegistry.newRegistration().add(entry, "test-config-entry")) {
+ // The admin can see refs/meta/config and hence has the READ_CONFIG permission.
+ requestScopeOperations.setApiUser(admin.id());
+ ConfigInfo configInfo = getConfig();
+ assertThat(configInfo.pluginConfig).isNotNull();
+ assertThat(configInfo.pluginConfig).isNotEmpty();
+ }
+ }
+
+ @Test
+ public void pluginConfigsNotReturnedWhenRefsMetaConfigNotReadable() throws Exception {
+ ProjectConfigEntry entry = new ProjectConfigEntry("enabled", "true");
+ try (Registration ignored =
+ extensionRegistry.newRegistration().add(entry, "test-config-entry")) {
+ // This user cannot see refs/meta/config and hence does not have the READ_CONFIG permission.
+ requestScopeOperations.setApiUser(user.id());
+ ConfigInfo configInfo = getConfig();
+ assertThat(configInfo.pluginConfig).isNull();
+ }
+ }
+
+ @Test
public void noCommentlinksByDefault() throws Exception {
assertThat(getConfig().commentlinks).isEmpty();
}
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java b/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java
index a8149ce..1532b33 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/AttentionSetIT.java
@@ -240,6 +240,50 @@
assertThat(attentionSet.reason()).isEqualTo("Change was submitted");
}
+ /**
+ * There is currently a bug that adds the person who submitted the change as reviewer, which in
+ * turn adds them to the attention set. This test ensures this doesn't happen.
+ */
+ @Test
+ public void submitDoesNotAddReviewersToAttentionSet() throws Exception {
+ PushOneCommit.Result r = createChange("refs/heads/master", "file1", "content");
+
+ // Someone else approves, because if admin reviews, they will be added to the reviewers (and the
+ // bug won't be reproduced).
+ requestScopeOperations.setApiUser(accountCreator.admin2().id());
+ change(r).current().review(ReviewInput.approve().addUserToAttentionSet(user.email(), "reason"));
+
+ requestScopeOperations.setApiUser(admin.id());
+
+ change(r).attention(admin.email()).remove(new AttentionSetInput("remove"));
+ change(r).current().submit();
+
+ // Attention set updates that relate to the admin (the person who replied) are filtered out.
+ AttentionSetUpdate attentionSet =
+ Iterables.getOnlyElement(getAttentionSetUpdatesForUser(r, admin));
+
+ assertThat(attentionSet.account()).isEqualTo(admin.id());
+ assertThat(attentionSet.operation()).isEqualTo(AttentionSetUpdate.Operation.REMOVE);
+ assertThat(attentionSet.reason()).isEqualTo("remove");
+
+ change(r).addReviewer(user.email());
+ }
+
+ @Test
+ public void addedReviewersAreAddedToAttentionSetOnMergedChanges() throws Exception {
+ PushOneCommit.Result r = createChange();
+ change(r).current().review(ReviewInput.approve());
+ change(r).current().submit();
+
+ change(r).addReviewer(user.email());
+ AttentionSetUpdate attentionSet =
+ Iterables.getOnlyElement(getAttentionSetUpdatesForUser(r, user));
+
+ assertThat(attentionSet.account()).isEqualTo(user.id());
+ assertThat(attentionSet.operation()).isEqualTo(AttentionSetUpdate.Operation.ADD);
+ assertThat(attentionSet.reason()).isEqualTo("Reviewer was added");
+ }
+
@Test
public void reviewersAddedAndRemovedFromAttentionSet() throws Exception {
PushOneCommit.Result r = createChange();
