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();