jackrabbit-oak-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Tobias Bocanegra <tri...@adobe.com>
Subject Re: svn commit: r1556370 - in /jackrabbit/oak/trunk: oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/ oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/permission/ oak-jcr/
Date Wed, 08 Jan 2014 18:54:28 GMT
Hi,

also, I think you removed some optimizations we did for the permission
evaluation (which are not directly related to the global cache). I'll
revert your changes and "disabled" the global cache instead.

regards, toby

On Wed, Jan 8, 2014 at 10:44 AM, Angela Schreiber <anchela@adobe.com> wrote:
> hi jukka
>
> maybe i missed the corresponding discussion on the list as i busy with
> other
> stuff lately... but the changes below don't 'disable' the cache as you
> claim
> in the commit message but rather remove it altogether.
>
> did you discuss this with tobi prior to the modifications? i think he
> knows
> best would could go wrong here and how to fix it.
> i would have appreciated if you would really just have disabled it in
> the evaluation.
>
> in case this is not yet done: can you please create an issue to get the
> cache fixed instead of just removing it?
>
> thanks
> angela
>
> On 07/01/14 22:51, "jukka@apache.org" <jukka@apache.org> wrote:
>
>>Author: jukka
>>Date: Tue Jan  7 21:51:07 2014
>>New Revision: 1556370
>>
>>URL: http://svn.apache.org/r1556370
>>Log:
>>OAK-1247: Non-deterministic access control test failures
>>
>>Disable the PermissionEntryCache to make the test suite deterministic
>>
>>Modified:
>>
>>jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/secu
>>rity/authorization/AuthorizationConfigurationImpl.java
>>
>>jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/secu
>>rity/authorization/permission/CompiledPermissionImpl.java
>>
>>jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/secu
>>rity/authorization/permission/PermissionEntryProvider.java
>>
>>jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/secu
>>rity/authorization/permission/PermissionEntryProviderImpl.java
>>
>>jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/secu
>>rity/authorization/permission/PermissionHook.java
>>
>>jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/secu
>>rity/authorization/permission/PermissionProviderImpl.java
>>    jackrabbit/oak/trunk/oak-jcr/pom.xml
>>
>>Modified:
>>jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/secu
>>rity/authorization/AuthorizationConfigurationImpl.java
>>URL:
>>http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/o
>>rg/apache/jackrabbit/oak/security/authorization/AuthorizationConfiguration
>>Impl.java?rev=1556370&r1=1556369&r2=1556370&view=diff
>>==========================================================================
>>====
>>---
>>jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/secu
>>rity/authorization/AuthorizationConfigurationImpl.java (original)
>>+++
>>jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/secu
>>rity/authorization/AuthorizationConfigurationImpl.java Tue Jan  7
>>21:51:07 2014
>>@@ -32,7 +32,6 @@ import org.apache.jackrabbit.oak.plugins
>> import
>>org.apache.jackrabbit.oak.security.authorization.accesscontrol.AccessContr
>>olImporter;
>> import
>>org.apache.jackrabbit.oak.security.authorization.accesscontrol.AccessContr
>>olManagerImpl;
>> import
>>org.apache.jackrabbit.oak.security.authorization.accesscontrol.AccessContr
>>olValidatorProvider;
>>-import
>>org.apache.jackrabbit.oak.security.authorization.permission.PermissionEntr
>>yCache;
>> import
>>org.apache.jackrabbit.oak.security.authorization.permission.PermissionHook
>>;
>> import
>>org.apache.jackrabbit.oak.security.authorization.permission.PermissionProv
>>iderImpl;
>> import
>>org.apache.jackrabbit.oak.security.authorization.permission.PermissionStor
>>eValidatorProvider;
>>@@ -61,8 +60,6 @@ import com.google.common.collect.Immutab
>> @Service({AuthorizationConfiguration.class, SecurityConfiguration.class})
>> public class AuthorizationConfigurationImpl extends ConfigurationBase
>>implements AuthorizationConfiguration {
>>
>>-    private final PermissionEntryCache permissionEntryCache = new
>>PermissionEntryCache();
>>-
>>     public AuthorizationConfigurationImpl() {
>>         super();
>>     }
>>@@ -94,7 +91,7 @@ public class AuthorizationConfigurationI
>>     public List<? extends CommitHook> getCommitHooks(String
>>workspaceName) {
>>         return ImmutableList.of(
>>                 new VersionablePathHook(workspaceName),
>>-                new PermissionHook(workspaceName,
>>getRestrictionProvider(), permissionEntryCache));
>>+                new PermissionHook(workspaceName,
>>getRestrictionProvider()));
>>     }
>>
>>     @Override
>>@@ -131,7 +128,7 @@ public class AuthorizationConfigurationI
>>     @Nonnull
>>     @Override
>>     public PermissionProvider getPermissionProvider(Root root, String
>>workspaceName, Set<Principal> principals) {
>>-        return new PermissionProviderImpl(root, workspaceName,
>>principals, this, permissionEntryCache.createLocalCache());
>>+        return new PermissionProviderImpl(root, workspaceName,
>>principals, this);
>>     }
>>
>> }
>>
>>Modified:
>>jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/secu
>>rity/authorization/permission/CompiledPermissionImpl.java
>>URL:
>>http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/o
>>rg/apache/jackrabbit/oak/security/authorization/permission/CompiledPermiss
>>ionImpl.java?rev=1556370&r1=1556369&r2=1556370&view=diff
>>==========================================================================
>>====
>>---
>>jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/secu
>>rity/authorization/permission/CompiledPermissionImpl.java (original)
>>+++
>>jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/secu
>>rity/authorization/permission/CompiledPermissionImpl.java Tue Jan  7
>>21:51:07 2014
>>@@ -86,8 +86,7 @@ final class CompiledPermissionImpl imple
>>
>>     private PrivilegeBitsProvider bitsProvider;
>>
>>-    private CompiledPermissionImpl(@Nonnull PermissionEntryCache.Local
>>cache,
>>-                                   @Nonnull Set<Principal> principals,
>>+    private CompiledPermissionImpl(@Nonnull Set<Principal> principals,
>>                                    @Nonnull ImmutableRoot root, @Nonnull
>>String workspaceName,
>>                                    @Nonnull RestrictionProvider
>>restrictionProvider,
>>                                    @Nonnull Set<String> readPaths) {
>>@@ -109,20 +108,19 @@ final class CompiledPermissionImpl imple
>>             }
>>         }
>>
>>-        userStore = new PermissionEntryProviderImpl(store, cache,
>>userNames);
>>-        groupStore = new PermissionEntryProviderImpl(store, cache,
>>groupNames);
>>+        userStore = new PermissionEntryProviderImpl(store, userNames);
>>+        groupStore = new PermissionEntryProviderImpl(store, groupNames);
>>     }
>>
>>     static CompiledPermissions create(@Nonnull ImmutableRoot root,
>>@Nonnull String workspaceName,
>>                                       @Nonnull Set<Principal> principals,
>>-                                      @Nonnull
>>AuthorizationConfiguration acConfig,
>>-                                      @Nonnull
>>PermissionEntryCache.Local cache) {
>>+                                      @Nonnull
>>AuthorizationConfiguration acConfig) {
>>         Tree permissionsTree = PermissionUtil.getPermissionsRoot(root,
>>workspaceName);
>>         if (!permissionsTree.exists() || principals.isEmpty()) {
>>             return NoPermissions.getInstance();
>>         } else {
>>             Set<String> readPaths =
>>acConfig.getParameters().getConfigValue(PARAM_READ_PATHS,
>>DEFAULT_READ_PATHS);
>>-            return new CompiledPermissionImpl(cache, principals, root,
>>workspaceName, acConfig.getRestrictionProvider(), readPaths);
>>+            return new CompiledPermissionImpl(principals, root,
>>workspaceName, acConfig.getRestrictionProvider(), readPaths);
>>         }
>>     }
>>
>>@@ -132,8 +130,6 @@ final class CompiledPermissionImpl imple
>>         this.root = root;
>>         this.bitsProvider = new PrivilegeBitsProvider(root);
>>         store.flush(root);
>>-        userStore.flush();
>>-        groupStore.flush();
>>     }
>>
>>     @Override
>>
>>Modified:
>>jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/secu
>>rity/authorization/permission/PermissionEntryProvider.java
>>URL:
>>http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/o
>>rg/apache/jackrabbit/oak/security/authorization/permission/PermissionEntry
>>Provider.java?rev=1556370&r1=1556369&r2=1556370&view=diff
>>==========================================================================
>>====
>>---
>>jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/secu
>>rity/authorization/permission/PermissionEntryProvider.java (original)
>>+++
>>jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/secu
>>rity/authorization/permission/PermissionEntryProvider.java Tue Jan  7
>>21:51:07 2014
>>@@ -38,5 +38,4 @@ interface PermissionEntryProvider {
>>     @Nonnull
>>     Collection<PermissionEntry> getEntries(@Nonnull String
>>accessControlledPath);
>>
>>-    void flush();
>>-}
>>\ No newline at end of file
>>+}
>>
>>Modified:
>>jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/secu
>>rity/authorization/permission/PermissionEntryProviderImpl.java
>>URL:
>>http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/o
>>rg/apache/jackrabbit/oak/security/authorization/permission/PermissionEntry
>>ProviderImpl.java?rev=1556370&r1=1556369&r2=1556370&view=diff
>>==========================================================================
>>====
>>---
>>jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/secu
>>rity/authorization/permission/PermissionEntryProviderImpl.java (original)
>>+++
>>jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/secu
>>rity/authorization/permission/PermissionEntryProviderImpl.java Tue Jan  7
>>21:51:07 2014
>>@@ -18,10 +18,7 @@ package org.apache.jackrabbit.oak.securi
>>
>> import java.util.Collection;
>> import java.util.Collections;
>>-import java.util.HashMap;
>>-import java.util.HashSet;
>> import java.util.Iterator;
>>-import java.util.Map;
>> import java.util.Set;
>> import java.util.TreeSet;
>>
>>@@ -39,98 +36,37 @@ import com.google.common.collect.Iterato
>>  */
>> class PermissionEntryProviderImpl implements PermissionEntryProvider {
>>
>>-    private static final long MAX_SIZE = 250; // TODO define size or
>>make configurable
>>-
>>     private final Set<String> principalNames;
>>
>>-    private final Set<String> existingNames = new HashSet<String>();
>>-
>>-    private Map<String, Collection<PermissionEntry>> pathEntryMap;
>>-
>>     private final PermissionStore store;
>>
>>-    private final PermissionEntryCache.Local cache;
>>-
>>-    PermissionEntryProviderImpl(@Nonnull PermissionStore store, @Nonnull
>>PermissionEntryCache.Local cache,
>>+    PermissionEntryProviderImpl(@Nonnull PermissionStore store,
>>                                 @Nonnull Set<String> principalNames) {
>>         this.store = store;
>>-        this.cache = cache;
>>         this.principalNames =
>>Collections.unmodifiableSet(principalNames);
>>-        init();
>>-    }
>>-
>>-    private void init() {
>>-        long cnt = 0;
>>-        existingNames.clear();
>>-        for (String name: principalNames) {
>>-            if (cnt > MAX_SIZE) {
>>-                if (cache.hasEntries(store, name)) {
>>-                    existingNames.add(name);
>>-                }
>>-            } else {
>>-                long n = cache.getNumEntries(store, name);
>>-                cnt+= n;
>>-                if (n > 0) {
>>-                    existingNames.add(name);
>>-                }
>>-            }
>>-        }
>>-        if (cnt < MAX_SIZE) {
>>-            // cache all entries of all principals
>>-            pathEntryMap = new HashMap<String,
>>Collection<PermissionEntry>>();
>>-            for (String name: principalNames) {
>>-                cache.load(store, pathEntryMap, name);
>>-            }
>>-        } else {
>>-            pathEntryMap = null;
>>-        }
>>-    }
>>-
>>-    public void flush() {
>>-        cache.flush(principalNames);
>>-        init();
>>     }
>>
>>     @Nonnull
>>     public Iterator<PermissionEntry> getEntryIterator(@Nonnull
>>EntryPredicate predicate) {
>>-        if (existingNames.isEmpty()) {
>>-            return Iterators.emptyIterator();
>>-        } else {
>>-            return new EntryIterator(predicate);
>>-        }
>>+        return new EntryIterator(predicate);
>>     }
>>
>>     @Nonnull
>>     public Collection<PermissionEntry> getEntries(@Nonnull Tree
>>accessControlledTree) {
>>-        if (existingNames.isEmpty()) {
>>-            return Collections.emptyList();
>>-        } else if (pathEntryMap != null) {
>>-            Collection<PermissionEntry> entries =
>>pathEntryMap.get(accessControlledTree.getPath());
>>-            return (entries != null) ? entries :
>>Collections.<PermissionEntry>emptyList();
>>+        if
>>(accessControlledTree.hasChild(AccessControlConstants.REP_POLICY)) {
>>+            return getEntries(accessControlledTree.getPath());
>>         } else {
>>-            return
>>(accessControlledTree.hasChild(AccessControlConstants.REP_POLICY)) ?
>>-                    loadEntries(accessControlledTree.getPath()) :
>>-                    Collections.<PermissionEntry>emptyList();
>>+            return Collections.<PermissionEntry>emptyList();
>>         }
>>     }
>>
>>     @Nonnull
>>     public Collection<PermissionEntry> getEntries(@Nonnull String path) {
>>-        if (existingNames.isEmpty()) {
>>-            return Collections.emptyList();
>>-        } else if (pathEntryMap != null) {
>>-            Collection<PermissionEntry> entries = pathEntryMap.get(path);
>>-            return (entries != null) ? entries :
>>Collections.<PermissionEntry>emptyList();
>>-        } else {
>>-            return loadEntries(path);
>>-        }
>>-    }
>>-
>>-    @Nonnull
>>-    private Collection<PermissionEntry> loadEntries(@Nonnull String
>>path) {
>>         Collection<PermissionEntry> ret = new TreeSet<PermissionEntry>();
>>-        for (String name: existingNames) {
>>-            cache.load(store, ret, name, path);
>>+        for (String name: principalNames) {
>>+            // todo: conditionally load entries if too many
>>+            PrincipalPermissionEntries ppe = store.load(name);
>>+            ret.addAll(ppe.getEntries(path));
>>         }
>>         return ret;
>>     }
>>
>>Modified:
>>jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/secu
>>rity/authorization/permission/PermissionHook.java
>>URL:
>>http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/o
>>rg/apache/jackrabbit/oak/security/authorization/permission/PermissionHook.
>>java?rev=1556370&r1=1556369&r2=1556370&view=diff
>>==========================================================================
>>====
>>---
>>jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/secu
>>rity/authorization/permission/PermissionHook.java (original)
>>+++
>>jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/secu
>>rity/authorization/permission/PermissionHook.java Tue Jan  7 21:51:07 2014
>>@@ -84,7 +84,6 @@ public class PermissionHook implements P
>>
>>     private final RestrictionProvider restrictionProvider;
>>     private final String workspaceName;
>>-    private final PermissionEntryCache cache;
>>
>>     private NodeBuilder permissionRoot;
>>     private PrivilegeBitsProvider bitsProvider;
>>@@ -96,10 +95,9 @@ public class PermissionHook implements P
>>     private Map<String, Acl> modified = new HashMap<String, Acl>();
>>     private Map<String, Acl> deleted = new HashMap<String, Acl>();
>>
>>-    public PermissionHook(String workspaceName, RestrictionProvider
>>restrictionProvider, PermissionEntryCache cache) {
>>+    public PermissionHook(String workspaceName, RestrictionProvider
>>restrictionProvider) {
>>         this.workspaceName = workspaceName;
>>         this.restrictionProvider = restrictionProvider;
>>-        this.cache = cache;
>>     }
>>
>>     @Nonnull
>>@@ -128,7 +126,6 @@ public class PermissionHook implements P
>>         for (Map.Entry<String, Acl> entry : modified.entrySet()) {
>>             entry.getValue().update(principalNames);
>>         }
>>-        cache.flush(principalNames);
>>     }
>>
>>     @Nonnull
>>
>>Modified:
>>jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/secu
>>rity/authorization/permission/PermissionProviderImpl.java
>>URL:
>>http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/o
>>rg/apache/jackrabbit/oak/security/authorization/permission/PermissionProvi
>>derImpl.java?rev=1556370&r1=1556369&r2=1556370&view=diff
>>==========================================================================
>>====
>>---
>>jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/secu
>>rity/authorization/permission/PermissionProviderImpl.java (original)
>>+++
>>jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/secu
>>rity/authorization/permission/PermissionProviderImpl.java Tue Jan  7
>>21:51:07 2014
>>@@ -58,8 +58,7 @@ public class PermissionProviderImpl impl
>>     private ImmutableRoot immutableRoot;
>>
>>     public PermissionProviderImpl(@Nonnull Root root, @Nonnull String
>>workspaceName, @Nonnull Set<Principal> principals,
>>-                                  @Nonnull AuthorizationConfiguration
>>acConfig,
>>-                                  @Nonnull PermissionEntryCache.Local
>>cache) {
>>+                                  @Nonnull AuthorizationConfiguration
>>acConfig) {
>>         this.root = root;
>>         this.workspaceName = workspaceName;
>>         this.acConfig = acConfig;
>>@@ -69,7 +68,7 @@ public class PermissionProviderImpl impl
>>         if (principals.contains(SystemPrincipal.INSTANCE) ||
>>isAdmin(principals)) {
>>             compiledPermissions = AllPermissions.getInstance();
>>         } else {
>>-            compiledPermissions =
>>CompiledPermissionImpl.create(immutableRoot, workspaceName, principals,
>>acConfig, cache);
>>+            compiledPermissions =
>>CompiledPermissionImpl.create(immutableRoot, workspaceName, principals,
>>acConfig);
>>         }
>>     }
>>
>>
>>Modified: jackrabbit/oak/trunk/oak-jcr/pom.xml
>>URL:
>>http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-jcr/pom.xml?rev=1556
>>370&r1=1556369&r2=1556370&view=diff
>>==========================================================================
>>====
>>--- jackrabbit/oak/trunk/oak-jcr/pom.xml (original)
>>+++ jackrabbit/oak/trunk/oak-jcr/pom.xml Tue Jan  7 21:51:07 2014
>>@@ -117,6 +117,10 @@
>>
>>org.apache.jackrabbit.oak.jcr.security.authorization.CopyTest#testCopyInvi
>>sibleAcContent       <!-- OAK-920 -->
>>
>>
>>org.apache.jackrabbit.oak.jcr.security.authorization.SessionMoveTest#testM
>>oveAddSubTreeWithRestriction <!-- OAK-1223 -->
>>+
>>org.apache.jackrabbit.oak.jcr.security.authorization.SessionMoveTest#testM
>>oveAndRemoveSubTree3         <!-- OAK-710 -->
>>+
>>org.apache.jackrabbit.oak.jcr.security.authorization.SessionMoveTest#testM
>>oveAndAddSubTree3            <!-- OAK-710 -->
>>+
>>org.apache.jackrabbit.oak.jcr.security.authorization.SessionMoveTest#testM
>>oveAndAddProperty2           <!-- OAK-710 -->
>>+
>>org.apache.jackrabbit.oak.jcr.security.authorization.SessionMoveTest#testM
>>oveAndRemoveProperty2        <!-- OAK-710 -->
>>
>>       <!-- User Management -->
>>
>>org.apache.jackrabbit.oak.jcr.security.user.RefreshTest#testAuthorizableGe
>>tProperty            <!-- OAK-1124 -->
>>
>>
>

Mime
View raw message