### Eclipse Workspace Patch 1.0 #P oak-core Index: src/main/java/org/apache/jackrabbit/oak/security/authorization/permission/PermissionEntryProviderImpl.java =================================================================== --- src/main/java/org/apache/jackrabbit/oak/security/authorization/permission/PermissionEntryProviderImpl.java (revision 1655988) +++ src/main/java/org/apache/jackrabbit/oak/security/authorization/permission/PermissionEntryProviderImpl.java (working copy) @@ -24,33 +24,54 @@ import java.util.Map; import java.util.Set; import java.util.TreeSet; + import javax.annotation.Nonnull; import com.google.common.base.Strings; import com.google.common.collect.Iterators; +import com.google.common.math.LongMath; + import org.apache.jackrabbit.commons.iterator.AbstractLazyIterator; import org.apache.jackrabbit.oak.api.Tree; import org.apache.jackrabbit.oak.spi.security.ConfigurationParameters; import org.apache.jackrabbit.oak.spi.security.authorization.accesscontrol.AccessControlConstants; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; class PermissionEntryProviderImpl implements PermissionEntryProvider { + + /** + * default logger + */ + private static final Logger log = LoggerFactory.getLogger(PermissionEntryProviderImpl.class); public static final String EAGER_CACHE_SIZE_PARAM = "eagerCacheSize"; private static final long DEFAULT_SIZE = 250; + /** + * The set of principal names for which this {@code PermissionEntryProvider} + * has been created. + */ private final Set principalNames; + /** + * The set of principal names for which the store contains any permission + * entries. This set is equals or just a subset of the {@code principalNames} + * defined above. The methods collecting the entries will shortcut in case + * this set is empty and thus no permission entries exist for the specified + * set of principal. + */ private final Set existingNames = new HashSet(); - private Map> pathEntryMap; - private final PermissionStore store; private final PermissionEntryCache cache; private final long maxSize; + private Map> pathEntryMap; + PermissionEntryProviderImpl(@Nonnull PermissionStore store, @Nonnull PermissionEntryCache cache, @Nonnull Set principalNames, @Nonnull ConfigurationParameters options) { this.store = store; @@ -63,17 +84,37 @@ private void init() { long cnt = 0; existingNames.clear(); - for (String name: principalNames) { + for (String name : principalNames) { + /* + Calculate the total number of permission entries (cnt) defined for the + given set of principals in order to be able to determine if the cache + should be loaded upfront. + Note however that cache.getNumEntries (n) may return Long.MAX_VALUE + if the underlying implementation does not know the exact value, and + the child node count is higher than maxSize (see OAK-2465). + */ long n = cache.getNumEntries(store, name, maxSize); - cnt+= n; + if (Long.MAX_VALUE == n || Long.MAX_VALUE == cnt) { + cnt = Long.MAX_VALUE; + } else { + try { + cnt = LongMath.checkedAdd(cnt,n); + } catch (ArithmeticException ae) { + log.warn("Long overflow while calculate the total number of permission entries"); + cnt = Long.MAX_VALUE; + } + } + // if cache.getNumEntries (n) returns a number bigger than 0, we + // add this principal name to the 'existingNames' if (n > 0) { existingNames.add(name); } } - if (cnt < maxSize) { - // cache all entries of all principals + if (cnt > 0 && cnt < maxSize) { + // the total number of entries is smaller that maxSize, so we can + // cache all entries of all principals having any entries right away pathEntryMap = new HashMap>(); - for (String name: principalNames) { + for (String name : existingNames) { cache.load(store, pathEntryMap, name); } } else { @@ -124,7 +165,7 @@ @Nonnull private Collection loadEntries(@Nonnull String path) { Collection ret = new TreeSet(); - for (String name: existingNames) { + for (String name : existingNames) { cache.load(store, ret, name, path); } return ret; Index: src/test/java/org/apache/jackrabbit/oak/security/authorization/permission/PermissionEntryProviderImplTest.java =================================================================== --- src/test/java/org/apache/jackrabbit/oak/security/authorization/permission/PermissionEntryProviderImplTest.java (revision 0) +++ src/test/java/org/apache/jackrabbit/oak/security/authorization/permission/PermissionEntryProviderImplTest.java (working copy) @@ -0,0 +1,167 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You 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 org.apache.jackrabbit.oak.security.authorization.permission; + +import java.lang.reflect.Field; +import java.util.Collection; +import java.util.HashSet; +import java.util.Map; +import java.util.Set; + +import javax.annotation.Nonnull; + +import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Iterators; +import com.google.common.collect.Sets; + +import junit.framework.Assert; + +import org.apache.jackrabbit.oak.spi.security.ConfigurationParameters; +import org.junit.Test; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotSame; + +public class PermissionEntryProviderImplTest { + + private final String GROUP_LONG_MAX = "groupLongMax"; + private final String GROUP_LONG_MAX_MINUS_10 = "groupLongMaxMinus10"; + private final String GROUP_50 = "group50"; + + /** + * @see OAK-2465 + */ + @Test + public void testInitLongOverflow() throws Exception { + MockPermissionStore store = new MockPermissionStore(); + PermissionEntryCache cache = new MockPermissionEntryCache(); + Set principalNames = ImmutableSet.of(GROUP_LONG_MAX); + + /* + create a new PermissionEntryProviderImpl to have it's #init() method + called, which may trigger the cache to be pre-filled if the max number + if entries is not exceeded -> in case of PermissionStore#getNumEntries + return Long.MAX_VALUE the cache should not be filled (-> the mock-cache + implementation will fail. + */ + PermissionEntryProviderImpl provider = new PermissionEntryProviderImpl(store, cache, principalNames, ConfigurationParameters.EMPTY); + Field existingNamesField = provider.getClass().getDeclaredField("existingNames"); + existingNamesField.setAccessible(true); + + // test that PermissionEntryProviderImpl.existingNames nevertheless is + // properly filled with all principal names for which permission entries exist + assertEquals(principalNames, existingNamesField.get(provider)); + assertNotSame(Iterators.emptyIterator(), provider.getEntryIterator(new EntryPredicate())); + + } + + /** + * @see OAK-2465 + */ + @Test + public void testInitLongOverflow2() throws Exception { + MockPermissionStore store = new MockPermissionStore(); + PermissionEntryCache cache = new MockPermissionEntryCache(); + Set principalNames = ImmutableSet.of(GROUP_LONG_MAX_MINUS_10, GROUP_50); + + /* + create a new PermissionEntryProviderImpl to have it's #init() method + called, which may trigger the cache to be pre-filled if the max number + if entries is not exceeded -> still counting up the number of permission + entries must deal with the fact that the counter may become bigger that + Long.MAX_VALUE + */ + PermissionEntryProviderImpl provider = new PermissionEntryProviderImpl(store, cache, principalNames, ConfigurationParameters.EMPTY); + Field existingNamesField = provider.getClass().getDeclaredField("existingNames"); + existingNamesField.setAccessible(true); + + assertEquals(principalNames, existingNamesField.get(provider)); + assertNotSame(Iterators.emptyIterator(), provider.getEntryIterator(new EntryPredicate())); + } + + /** + * @see OAK-2465 + */ + @Test + public void testExistingNamesAndLongOverFlow() throws Exception { + MockPermissionStore store = new MockPermissionStore(); + PermissionEntryCache cache = new MockPermissionEntryCache(); + Set principalNames = Sets.newHashSet(GROUP_LONG_MAX_MINUS_10, GROUP_50, "noEntries"); + + /* + same as before but principal-set contains a name for which not entries + exist -> the 'existingNames' set must properly reflect that + */ + PermissionEntryProviderImpl provider = new PermissionEntryProviderImpl(store, cache, principalNames, ConfigurationParameters.EMPTY); + Field existingNamesField = provider.getClass().getDeclaredField("existingNames"); + existingNamesField.setAccessible(true); + Set existingNames = (Set) existingNamesField.get(provider); + + + assertFalse(principalNames.equals(existingNames)); + assertEquals(2, existingNames.size()); + + principalNames.remove("noEntries"); + assertEquals(principalNames, existingNames); + } + + // Inner Classes + private class MockPermissionStore implements PermissionStore { + + @Override + public Collection load( + Collection entries, String principalName, + String path) { + return null; + } + + @Override + public void load(Map> entries, + String principalName) { + // ignore + } + + @Override + public PrincipalPermissionEntries load(String principalName) { + return new PrincipalPermissionEntries(principalName); + } + + @Override + public long getNumEntries(String principalName, long max) { + long cnt = 0; + if (GROUP_LONG_MAX_MINUS_10.equals(principalName)) { + cnt = Long.MAX_VALUE - 10; + } else if (GROUP_50.equals(principalName)) { + cnt = 50; + } else if (GROUP_LONG_MAX.equals(principalName)) { + cnt = Long.MAX_VALUE; + } + return cnt; + } + + } + + private class MockPermissionEntryCache extends PermissionEntryCache { + @Override + public void load(@Nonnull PermissionStore store, + @Nonnull Map> pathEntryMap, + @Nonnull String principalName) { + Assert.fail("The number of entries exceeds the max cache size"); + } + } +}