diff --git a/java/com/google/gerrit/server/extensions/webui/UiActions.java b/java/com/google/gerrit/server/extensions/webui/UiActions.java index d9397f450d..6714055218 100644 --- a/java/com/google/gerrit/server/extensions/webui/UiActions.java +++ b/java/com/google/gerrit/server/extensions/webui/UiActions.java @@ -18,6 +18,7 @@ import static com.google.gerrit.extensions.conditions.BooleanCondition.and; import static com.google.gerrit.extensions.conditions.BooleanCondition.or; import static java.util.stream.Collectors.toList; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Predicate; import com.google.common.collect.Streams; import com.google.gerrit.common.Nullable; @@ -38,8 +39,10 @@ import com.google.gerrit.server.permissions.PermissionBackendException; import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.Singleton; +import java.util.HashMap; import java.util.Iterator; import java.util.List; +import java.util.Map; import java.util.Objects; import java.util.Set; import org.slf4j.Logger; @@ -80,11 +83,26 @@ public class UiActions { descs.stream().flatMap(u -> Streams.stream(visibleCondition(u))), descs.stream().flatMap(u -> Streams.stream(enabledCondition(u)))) .collect(toList()); - permissionBackend.bulkEvaluateTest(conds); + + evaluatePermissionBackendConditions(permissionBackend, conds); return descs.stream().filter(u -> u.isVisible()).collect(toList()); } + @VisibleForTesting + static void evaluatePermissionBackendConditions( + PermissionBackend perm, List conds) { + Map dedupedConds = + new HashMap<>(conds.size()); + for (PermissionBackendCondition cond : conds) { + dedupedConds.put(cond, cond); + } + perm.bulkEvaluateTest(dedupedConds.keySet()); + for (PermissionBackendCondition cond : conds) { + cond.set(dedupedConds.get(cond).value()); + } + } + private static Iterable visibleCondition(Description u) { return u.getVisibleCondition().reduce().children(PermissionBackendCondition.class); } diff --git a/java/com/google/gerrit/server/permissions/PermissionBackend.java b/java/com/google/gerrit/server/permissions/PermissionBackend.java index 0cd9a754be..0c4f228a30 100644 --- a/java/com/google/gerrit/server/permissions/PermissionBackend.java +++ b/java/com/google/gerrit/server/permissions/PermissionBackend.java @@ -95,19 +95,15 @@ public abstract class PermissionBackend { } /** - * Bulk evaluate a collection of {@link PermissionBackendCondition} for view handling. + * Bulk evaluate a set of {@link PermissionBackendCondition} for view handling. * *

Overridden implementations should call {@link PermissionBackendCondition#set(boolean)} to * cache the result of {@code testOrFalse} in the condition for later evaluation. Caching the * result will bypass the usual invocation of {@code testOrFalse}. * - *

{@code conds} may contain duplicate entries (such as same user, resource, permission - * triplet). When duplicates exist, implementations should set a result into all instances to - * ensure {@code testOrFalse} does not get invoked during evaluation of the containing condition. - * * @param conds conditions to consider. */ - public void bulkEvaluateTest(Collection conds) { + public void bulkEvaluateTest(Set conds) { // Do nothing by default. The default implementation of PermissionBackendCondition // delegates to the appropriate testOrFalse method in PermissionBackend. } diff --git a/javatests/com/google/gerrit/server/extensions/webui/UiActionsTest.java b/javatests/com/google/gerrit/server/extensions/webui/UiActionsTest.java new file mode 100644 index 0000000000..c3d9ba5a0d --- /dev/null +++ b/javatests/com/google/gerrit/server/extensions/webui/UiActionsTest.java @@ -0,0 +1,123 @@ +// Copyright (C) 2018 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.server.extensions.webui; + +import static com.google.common.truth.Truth.assertThat; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; +import com.google.gerrit.extensions.restapi.AuthException; +import com.google.gerrit.reviewdb.client.Account; +import com.google.gerrit.server.CurrentUser; +import com.google.gerrit.server.account.GroupMembership; +import com.google.gerrit.server.permissions.PermissionBackend; +import com.google.gerrit.server.permissions.PermissionBackend.ForProject; +import com.google.gerrit.server.permissions.PermissionBackend.ForRef; +import com.google.gerrit.server.permissions.PermissionBackendCondition; +import com.google.gerrit.server.permissions.PermissionBackendException; +import com.google.gerrit.server.permissions.ProjectPermission; +import java.util.Collection; +import java.util.Set; +import org.easymock.EasyMock; +import org.junit.Test; + +public class UiActionsTest { + + private static class FakeForProject extends ForProject { + private boolean allowValueQueries = true; + + @Override + public CurrentUser user() { + return new CurrentUser() { + @Override + public GroupMembership getEffectiveGroups() { + throw new UnsupportedOperationException("not implemented"); + } + + @Override + public boolean isIdentifiedUser() { + return true; + } + + @Override + public Account.Id getAccountId() { + return new Account.Id(1); + } + }; + } + + @Override + public String resourcePath() { + return "/projects/test-project"; + } + + @Override + public ForProject user(CurrentUser user) { + throw new UnsupportedOperationException("not implemented"); + } + + @Override + public ForRef ref(String ref) { + throw new UnsupportedOperationException("not implemented"); + } + + @Override + public void check(ProjectPermission perm) throws AuthException, PermissionBackendException { + throw new UnsupportedOperationException("not implemented"); + } + + @Override + public Set test(Collection permSet) + throws PermissionBackendException { + assertThat(allowValueQueries).isTrue(); + return ImmutableSet.of(ProjectPermission.READ); + } + + private void disallowValueQueries() { + allowValueQueries = false; + } + } + + @Test + public void permissionBackendConditionEvaluationDeduplicatesAndBackfills() throws Exception { + FakeForProject forProject = new FakeForProject(); + + // Create three conditions, two of which are identical + PermissionBackendCondition cond1 = + (PermissionBackendCondition) forProject.testCond(ProjectPermission.CREATE_CHANGE); + PermissionBackendCondition cond2 = + (PermissionBackendCondition) forProject.testCond(ProjectPermission.READ); + PermissionBackendCondition cond3 = + (PermissionBackendCondition) forProject.testCond(ProjectPermission.CREATE_CHANGE); + + // Set up the Mock to expect a call of bulkEvaluateTest to only contain cond{1,2} since cond3 + // needs to be identified as duplicate and not called out explicitly. + PermissionBackend permissionBackendMock = EasyMock.createMock(PermissionBackend.class); + permissionBackendMock.bulkEvaluateTest(ImmutableSet.of(cond1, cond2)); + EasyMock.replay(permissionBackendMock); + + UiActions.evaluatePermissionBackendConditions( + permissionBackendMock, ImmutableList.of(cond1, cond2, cond3)); + + // Disallow queries for value to ensure that cond3 (previously left behind) is backfilled with + // the value of cond1 and issues no additional call to PermissionBackend. + forProject.disallowValueQueries(); + + // Assert the values of all conditions + assertThat(cond1.value()).isFalse(); + assertThat(cond2.value()).isTrue(); + assertThat(cond3.value()).isFalse(); + } +}