anchela commented on code in PR #1128: URL: https://github.com/apache/jackrabbit-oak/pull/1128#discussion_r1346059752
########## oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/PrincipalCacheConflictHandler.java: ########## @@ -0,0 +1,106 @@ +/* + * 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.user; + +import org.apache.jackrabbit.oak.api.PropertyState; +import org.apache.jackrabbit.oak.api.Type; +import org.apache.jackrabbit.oak.plugins.commit.DefaultThreeWayConflictHandler; +import org.apache.jackrabbit.oak.plugins.memory.PropertyBuilder; +import org.apache.jackrabbit.oak.spi.state.NodeBuilder; +import org.jetbrains.annotations.NotNull; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * The {@code RepMembersConflictHandler} takes care of merging the {@code rep:expiration} property + * during parallel updates. + *<p> + * The conflict handler deals with the following conflicts: + * <ul> + * <li>{@code addExistingProperty} : {@code Resolution.IGNORED}, We should not have add conflints, since the {@code rep:{@code rep:expiration}} node is created with the user</li> + * <li>{@code changeDeletedProperty}: {@code Resolution.IGNORED},</li> + * <li>{@code changeChangedProperty}: {@code Resolution.MERGED}, the properties with higher {@code rep:expiration} get merged</li> + * <li>{@code deleteChangedProperty}: {@code Resolution.IGNORED} .</li> + * <li>{@code deleteDeletedProperty}: {@code Resolution.IGNORED}.</li> + * <li>{@code changeDeletedNode} : {@code Resolution.IGNORED}, .</li> + * <li>{@code deleteChangedNode} : {@code Resolution.IGNORED}, </li> + * <li>{@code deleteDeletedNode} : {@code Resolution.IGNORED}.</li> + * </ul> + */ + +public class PrincipalCacheConflictHandler extends DefaultThreeWayConflictHandler { + + private static final Logger LOG = LoggerFactory.getLogger(PrincipalCacheConflictHandler.class); + + protected static final String REP_EXPIRATION = "rep:expiration"; + protected static final String REP_CACHE = "rep:cache"; Review Comment: duplicate constant -> i believe this can be replace by implementing CacheConstants ########## oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/PrincipalCacheConflictHandler.java: ########## @@ -0,0 +1,106 @@ +/* + * 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.user; + +import org.apache.jackrabbit.oak.api.PropertyState; +import org.apache.jackrabbit.oak.api.Type; +import org.apache.jackrabbit.oak.plugins.commit.DefaultThreeWayConflictHandler; +import org.apache.jackrabbit.oak.plugins.memory.PropertyBuilder; +import org.apache.jackrabbit.oak.spi.state.NodeBuilder; +import org.jetbrains.annotations.NotNull; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * The {@code RepMembersConflictHandler} takes care of merging the {@code rep:expiration} property + * during parallel updates. + *<p> + * The conflict handler deals with the following conflicts: + * <ul> + * <li>{@code addExistingProperty} : {@code Resolution.IGNORED}, We should not have add conflints, since the {@code rep:{@code rep:expiration}} node is created with the user</li> + * <li>{@code changeDeletedProperty}: {@code Resolution.IGNORED},</li> + * <li>{@code changeChangedProperty}: {@code Resolution.MERGED}, the properties with higher {@code rep:expiration} get merged</li> + * <li>{@code deleteChangedProperty}: {@code Resolution.IGNORED} .</li> + * <li>{@code deleteDeletedProperty}: {@code Resolution.IGNORED}.</li> + * <li>{@code changeDeletedNode} : {@code Resolution.IGNORED}, .</li> + * <li>{@code deleteChangedNode} : {@code Resolution.IGNORED}, </li> + * <li>{@code deleteDeletedNode} : {@code Resolution.IGNORED}.</li> + * </ul> + */ + +public class PrincipalCacheConflictHandler extends DefaultThreeWayConflictHandler { Review Comment: the conflict-handler can be package protected, right? it's only referenced by the UserConfigurationImpl and thus doesn't need to be public. i would also consider if CacheConflictHandler would be sufficient accurate as a name as the other classes related to the principal-cache are name 'CacheConstants' and 'CacheValidatorProvider'. WDYT? ########## oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/PrincipalCacheConflictHandler.java: ########## @@ -0,0 +1,106 @@ +/* + * 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.user; + +import org.apache.jackrabbit.oak.api.PropertyState; +import org.apache.jackrabbit.oak.api.Type; +import org.apache.jackrabbit.oak.plugins.commit.DefaultThreeWayConflictHandler; +import org.apache.jackrabbit.oak.plugins.memory.PropertyBuilder; +import org.apache.jackrabbit.oak.spi.state.NodeBuilder; +import org.jetbrains.annotations.NotNull; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * The {@code RepMembersConflictHandler} takes care of merging the {@code rep:expiration} property + * during parallel updates. + *<p> + * The conflict handler deals with the following conflicts: + * <ul> + * <li>{@code addExistingProperty} : {@code Resolution.IGNORED}, We should not have add conflints, since the {@code rep:{@code rep:expiration}} node is created with the user</li> + * <li>{@code changeDeletedProperty}: {@code Resolution.IGNORED},</li> + * <li>{@code changeChangedProperty}: {@code Resolution.MERGED}, the properties with higher {@code rep:expiration} get merged</li> + * <li>{@code deleteChangedProperty}: {@code Resolution.IGNORED} .</li> + * <li>{@code deleteDeletedProperty}: {@code Resolution.IGNORED}.</li> + * <li>{@code changeDeletedNode} : {@code Resolution.IGNORED}, .</li> + * <li>{@code deleteChangedNode} : {@code Resolution.IGNORED}, </li> + * <li>{@code deleteDeletedNode} : {@code Resolution.IGNORED}.</li> + * </ul> + */ + +public class PrincipalCacheConflictHandler extends DefaultThreeWayConflictHandler { + + private static final Logger LOG = LoggerFactory.getLogger(PrincipalCacheConflictHandler.class); + + protected static final String REP_EXPIRATION = "rep:expiration"; Review Comment: duplicate constant -> i believe this can be replace by implementing CacheConstants ########## oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/PrincipalCacheConflictHandler.java: ########## @@ -0,0 +1,106 @@ +/* + * 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.user; + +import org.apache.jackrabbit.oak.api.PropertyState; +import org.apache.jackrabbit.oak.api.Type; +import org.apache.jackrabbit.oak.plugins.commit.DefaultThreeWayConflictHandler; +import org.apache.jackrabbit.oak.plugins.memory.PropertyBuilder; +import org.apache.jackrabbit.oak.spi.state.NodeBuilder; +import org.jetbrains.annotations.NotNull; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * The {@code RepMembersConflictHandler} takes care of merging the {@code rep:expiration} property + * during parallel updates. + *<p> + * The conflict handler deals with the following conflicts: + * <ul> + * <li>{@code addExistingProperty} : {@code Resolution.IGNORED}, We should not have add conflints, since the {@code rep:{@code rep:expiration}} node is created with the user</li> Review Comment: typo in 'conflints' ########## oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserPrincipalProvider.java: ########## @@ -75,7 +75,7 @@ class UserPrincipalProvider implements PrincipalProvider { static final String PARAM_CACHE_EXPIRATION = "cacheExpiration"; static final long EXPIRATION_NO_CACHE = 0; - private static final long MEMBERSHIP_THRESHOLD = 0; + protected static final long MEMBERSHIP_THRESHOLD = 0; Review Comment: instead of making the constant protected -> move it to `PrincipalCommitterThread` as it is no longer used here in the `UserPrincipalProvider` ########## oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserPrincipalProviderCommitterProvider.java: ########## @@ -0,0 +1,59 @@ +/* + * 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.user; + +import org.apache.jackrabbit.oak.api.Root; +import org.apache.jackrabbit.oak.api.Tree; +import org.jetbrains.annotations.NotNull; + +import java.security.Principal; +import java.util.HashMap; +import java.util.Set; + +public class UserPrincipalProviderCommitterProvider { + + static UserPrincipalProviderCommitterProvider instance = null; + static HashMap<String, PrincipalCommitterThread> committerThreadMap = new HashMap<>(); + + public static UserPrincipalProviderCommitterProvider getInstance() { + if (instance == null) { + instance = new UserPrincipalProviderCommitterProvider(); + } + return instance; + } + + public synchronized PrincipalCommitterThread cacheGroups(@NotNull Tree authorizableNode, @NotNull Set<Principal> groupPrincipals, long expiration, Root root) { Review Comment: - return value annotation with `@Nullable` - Root should be annotated with `@NotNull` - the method doesn't need to be public as it is only consumed by the userprincpialprovider -> package private ########## oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserPrincipalProviderCommitterProvider.java: ########## @@ -0,0 +1,59 @@ +/* + * 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.user; + +import org.apache.jackrabbit.oak.api.Root; +import org.apache.jackrabbit.oak.api.Tree; +import org.jetbrains.annotations.NotNull; + +import java.security.Principal; +import java.util.HashMap; +import java.util.Set; + +public class UserPrincipalProviderCommitterProvider { + + static UserPrincipalProviderCommitterProvider instance = null; + static HashMap<String, PrincipalCommitterThread> committerThreadMap = new HashMap<>(); + + public static UserPrincipalProviderCommitterProvider getInstance() { Review Comment: return value annotation with `@Nullable` ########## oak-core/src/test/java/org/apache/jackrabbit/oak/AbstractSecurityTest.java: ########## @@ -89,8 +89,8 @@ public abstract class AbstractSecurityTest { protected Root root; protected QueryEngineSettings querySettings; - private final RootProvider rootProvider = new RootProviderService(); - private final TreeProvider treeProvider = new TreeProviderService(); + protected final RootProvider rootProvider = new RootProviderService(); + protected final TreeProvider treeProvider = new TreeProviderService(); Review Comment: please use getTreeProvider() in the conflict-handler test instead of altering the access modifier. ########## oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserPrincipalProviderCommitterProvider.java: ########## @@ -0,0 +1,59 @@ +/* + * 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.user; + +import org.apache.jackrabbit.oak.api.Root; +import org.apache.jackrabbit.oak.api.Tree; +import org.jetbrains.annotations.NotNull; + +import java.security.Principal; +import java.util.HashMap; +import java.util.Set; + +public class UserPrincipalProviderCommitterProvider { + + static UserPrincipalProviderCommitterProvider instance = null; + static HashMap<String, PrincipalCommitterThread> committerThreadMap = new HashMap<>(); + + public static UserPrincipalProviderCommitterProvider getInstance() { + if (instance == null) { + instance = new UserPrincipalProviderCommitterProvider(); + } + return instance; + } + + public synchronized PrincipalCommitterThread cacheGroups(@NotNull Tree authorizableNode, @NotNull Set<Principal> groupPrincipals, long expiration, Root root) { + String authorizableNodePath = authorizableNode.getPath(); + if (committerThreadMap.containsKey(authorizableNodePath)) { + // One thread is already committing. return null to inform the caller that doesn't have to wait for the commit to finish + return null; + } else { + PrincipalCommitterThread committerThread = new PrincipalCommitterThread(authorizableNode, groupPrincipals, expiration, root, committerThreadMap); Review Comment: see above regarding passing the thread-map around and have the removal in another class. ########## oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/PrincipalCommitterThread.java: ########## @@ -0,0 +1,91 @@ +/* + * 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.user; + +import org.apache.jackrabbit.guava.common.base.Joiner; +import org.apache.jackrabbit.guava.common.collect.Iterables; +import org.apache.jackrabbit.oak.api.CommitFailedException; +import org.apache.jackrabbit.oak.api.Root; +import org.apache.jackrabbit.oak.api.Tree; +import org.apache.jackrabbit.oak.commons.LongUtils; +import org.apache.jackrabbit.oak.plugins.tree.TreeUtil; +import org.apache.jackrabbit.util.Text; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import javax.jcr.AccessDeniedException; +import java.security.Principal; +import java.util.HashMap; +import java.util.Set; + +import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.MEMBERSHIP_THRESHOLD; + +public class PrincipalCommitterThread extends Thread { + Tree authorizableNode; + Set<Principal> groupPrincipals; + HashMap committerThreadMap; + long expiration; + Root root; + + private static final Logger log = LoggerFactory.getLogger(PrincipalCommitterThread.class); + + public PrincipalCommitterThread(Tree authorizableNode, Set<Principal> groupPrincipals, long expiration, Root root, HashMap committerThreadMap) { + this.authorizableNode = authorizableNode; + this.groupPrincipals = groupPrincipals; + this.committerThreadMap = committerThreadMap; + this.expiration = expiration; + this.root = root; + } + + @Override + public void run() { + super.run(); + // Do the commit + try { + root.refresh(); + + Tree cache = authorizableNode.getChild(CacheConstants.REP_CACHE); + if (!cache.exists()) { + if (groupPrincipals.size() <= MEMBERSHIP_THRESHOLD) { + log.debug("Omit cache creation for user without group membership at {}", authorizableNode.getPath()); + return; + } else { + log.debug("Create new group membership cache at {}", authorizableNode.getPath()); + cache = TreeUtil.addChild(authorizableNode, CacheConstants.REP_CACHE, CacheConstants.NT_REP_CACHE); + } + } + + cache.setProperty(CacheConstants.REP_EXPIRATION, LongUtils.calculateExpirationTime(expiration)); + String value = (groupPrincipals.isEmpty()) ? "" : Joiner.on(",").join(Iterables.transform(groupPrincipals, input -> Text.escape(input.getName()))); + cache.setProperty(CacheConstants.REP_GROUP_PRINCIPAL_NAMES, value); + + root.commit(CacheValidatorProvider.asCommitAttributes()); + log.debug("Cached group membership at {}", authorizableNode.getPath()); + + } catch (AccessDeniedException | CommitFailedException e) { + log.debug("Failed to cache group membership: {}", e.getMessage()); + } finally { + log.debug("Removing thread from committerThreadMap for {}", authorizableNode.getPath()); + committerThreadMap.remove(authorizableNode.getPath()); Review Comment: see comments elsewhere.... that looks troublesome to me. ########## oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/PrincipalCacheConflictHandler.java: ########## @@ -0,0 +1,106 @@ +/* + * 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.user; + +import org.apache.jackrabbit.oak.api.PropertyState; +import org.apache.jackrabbit.oak.api.Type; +import org.apache.jackrabbit.oak.plugins.commit.DefaultThreeWayConflictHandler; +import org.apache.jackrabbit.oak.plugins.memory.PropertyBuilder; +import org.apache.jackrabbit.oak.spi.state.NodeBuilder; +import org.jetbrains.annotations.NotNull; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * The {@code RepMembersConflictHandler} takes care of merging the {@code rep:expiration} property + * during parallel updates. + *<p> + * The conflict handler deals with the following conflicts: + * <ul> + * <li>{@code addExistingProperty} : {@code Resolution.IGNORED}, We should not have add conflints, since the {@code rep:{@code rep:expiration}} node is created with the user</li> + * <li>{@code changeDeletedProperty}: {@code Resolution.IGNORED},</li> + * <li>{@code changeChangedProperty}: {@code Resolution.MERGED}, the properties with higher {@code rep:expiration} get merged</li> + * <li>{@code deleteChangedProperty}: {@code Resolution.IGNORED} .</li> + * <li>{@code deleteDeletedProperty}: {@code Resolution.IGNORED}.</li> + * <li>{@code changeDeletedNode} : {@code Resolution.IGNORED}, .</li> + * <li>{@code deleteChangedNode} : {@code Resolution.IGNORED}, </li> + * <li>{@code deleteDeletedNode} : {@code Resolution.IGNORED}.</li> + * </ul> + */ + +public class PrincipalCacheConflictHandler extends DefaultThreeWayConflictHandler { + + private static final Logger LOG = LoggerFactory.getLogger(PrincipalCacheConflictHandler.class); + + protected static final String REP_EXPIRATION = "rep:expiration"; + protected static final String REP_CACHE = "rep:cache"; + + /** + * Create a new {@code ConflictHandler} which always returns + * {@code resolution}. + * + * @param resolution the resolution to return from all methods of this + * {@code ConflictHandler} instance. + */ + public PrincipalCacheConflictHandler(Resolution resolution) { Review Comment: as far as i can see this constructor is only used by the second constructor. so, i would suggest to either make it private or drop it altogether and simplify the code. ########## oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/PrincipalCacheConflictHandler.java: ########## @@ -0,0 +1,106 @@ +/* + * 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.user; + +import org.apache.jackrabbit.oak.api.PropertyState; +import org.apache.jackrabbit.oak.api.Type; +import org.apache.jackrabbit.oak.plugins.commit.DefaultThreeWayConflictHandler; +import org.apache.jackrabbit.oak.plugins.memory.PropertyBuilder; +import org.apache.jackrabbit.oak.spi.state.NodeBuilder; +import org.jetbrains.annotations.NotNull; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * The {@code RepMembersConflictHandler} takes care of merging the {@code rep:expiration} property + * during parallel updates. + *<p> + * The conflict handler deals with the following conflicts: + * <ul> + * <li>{@code addExistingProperty} : {@code Resolution.IGNORED}, We should not have add conflints, since the {@code rep:{@code rep:expiration}} node is created with the user</li> + * <li>{@code changeDeletedProperty}: {@code Resolution.IGNORED},</li> + * <li>{@code changeChangedProperty}: {@code Resolution.MERGED}, the properties with higher {@code rep:expiration} get merged</li> + * <li>{@code deleteChangedProperty}: {@code Resolution.IGNORED} .</li> + * <li>{@code deleteDeletedProperty}: {@code Resolution.IGNORED}.</li> + * <li>{@code changeDeletedNode} : {@code Resolution.IGNORED}, .</li> + * <li>{@code deleteChangedNode} : {@code Resolution.IGNORED}, </li> + * <li>{@code deleteDeletedNode} : {@code Resolution.IGNORED}.</li> + * </ul> + */ + +public class PrincipalCacheConflictHandler extends DefaultThreeWayConflictHandler { + + private static final Logger LOG = LoggerFactory.getLogger(PrincipalCacheConflictHandler.class); + + protected static final String REP_EXPIRATION = "rep:expiration"; + protected static final String REP_CACHE = "rep:cache"; + + /** + * Create a new {@code ConflictHandler} which always returns + * {@code resolution}. + * + * @param resolution the resolution to return from all methods of this + * {@code ConflictHandler} instance. + */ + public PrincipalCacheConflictHandler(Resolution resolution) { + super(resolution); + } + public PrincipalCacheConflictHandler() { + this(Resolution.IGNORED); Review Comment: see above. maybe just calling super(Resolution.IGNORED) and drop the first constructor. then, the constructor does not need to be public IMHO. i would recommend to make package protected... ########## oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/PrincipalCacheConflictHandler.java: ########## @@ -0,0 +1,106 @@ +/* + * 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.user; + +import org.apache.jackrabbit.oak.api.PropertyState; +import org.apache.jackrabbit.oak.api.Type; +import org.apache.jackrabbit.oak.plugins.commit.DefaultThreeWayConflictHandler; +import org.apache.jackrabbit.oak.plugins.memory.PropertyBuilder; +import org.apache.jackrabbit.oak.spi.state.NodeBuilder; +import org.jetbrains.annotations.NotNull; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * The {@code RepMembersConflictHandler} takes care of merging the {@code rep:expiration} property + * during parallel updates. + *<p> + * The conflict handler deals with the following conflicts: + * <ul> + * <li>{@code addExistingProperty} : {@code Resolution.IGNORED}, We should not have add conflints, since the {@code rep:{@code rep:expiration}} node is created with the user</li> + * <li>{@code changeDeletedProperty}: {@code Resolution.IGNORED},</li> + * <li>{@code changeChangedProperty}: {@code Resolution.MERGED}, the properties with higher {@code rep:expiration} get merged</li> + * <li>{@code deleteChangedProperty}: {@code Resolution.IGNORED} .</li> + * <li>{@code deleteDeletedProperty}: {@code Resolution.IGNORED}.</li> + * <li>{@code changeDeletedNode} : {@code Resolution.IGNORED}, .</li> + * <li>{@code deleteChangedNode} : {@code Resolution.IGNORED}, </li> + * <li>{@code deleteDeletedNode} : {@code Resolution.IGNORED}.</li> + * </ul> + */ + +public class PrincipalCacheConflictHandler extends DefaultThreeWayConflictHandler { + + private static final Logger LOG = LoggerFactory.getLogger(PrincipalCacheConflictHandler.class); + + protected static final String REP_EXPIRATION = "rep:expiration"; + protected static final String REP_CACHE = "rep:cache"; + + /** + * Create a new {@code ConflictHandler} which always returns + * {@code resolution}. + * + * @param resolution the resolution to return from all methods of this + * {@code ConflictHandler} instance. + */ + public PrincipalCacheConflictHandler(Resolution resolution) { + super(resolution); + } + public PrincipalCacheConflictHandler() { + this(Resolution.IGNORED); + } + + private Resolution resolveRepExpirationConflict(@NotNull NodeBuilder parent, @NotNull PropertyState ours, @NotNull PropertyState theirs, + PropertyState base) { + if ( REP_EXPIRATION.equals(ours.getName()) && REP_EXPIRATION.equals(theirs.getName()) ){ + + PropertyBuilder<Long> merged = PropertyBuilder.scalar(Type.LONG); + merged.setName(REP_EXPIRATION); + + //if base is bigger than ours and theirs, then use base. This should never happens + if ( base != null && + base.getValue(Type.LONG) > ours.getValue(Type.LONG) && + base.getValue(Type.LONG) > theirs.getValue(Type.LONG) ){ + merged.setValue(base.getValue(Type.LONG)); + return Resolution.MERGED; + } + + //if ours is bigger than theirs, then use ours + //otherwise use theirs + if ( ours.getValue(Type.LONG) > theirs.getValue(Type.LONG) ){ + merged.setValue(ours.getValue(Type.LONG)); + } else { + merged.setValue(theirs.getValue(Type.LONG)); + } + parent.setProperty(merged.getPropertyState()); + LOG.debug("Resolved conflict for property {} our value: {}, merged value: {}", REP_EXPIRATION, ours.getValue(Type.LONG), merged.getValue(0)); Review Comment: is there a reason you only log our value and not their value as well? ########## oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/PrincipalCacheConflictHandler.java: ########## @@ -0,0 +1,106 @@ +/* + * 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.user; + +import org.apache.jackrabbit.oak.api.PropertyState; +import org.apache.jackrabbit.oak.api.Type; +import org.apache.jackrabbit.oak.plugins.commit.DefaultThreeWayConflictHandler; +import org.apache.jackrabbit.oak.plugins.memory.PropertyBuilder; +import org.apache.jackrabbit.oak.spi.state.NodeBuilder; +import org.jetbrains.annotations.NotNull; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * The {@code RepMembersConflictHandler} takes care of merging the {@code rep:expiration} property + * during parallel updates. + *<p> + * The conflict handler deals with the following conflicts: + * <ul> + * <li>{@code addExistingProperty} : {@code Resolution.IGNORED}, We should not have add conflints, since the {@code rep:{@code rep:expiration}} node is created with the user</li> + * <li>{@code changeDeletedProperty}: {@code Resolution.IGNORED},</li> + * <li>{@code changeChangedProperty}: {@code Resolution.MERGED}, the properties with higher {@code rep:expiration} get merged</li> + * <li>{@code deleteChangedProperty}: {@code Resolution.IGNORED} .</li> + * <li>{@code deleteDeletedProperty}: {@code Resolution.IGNORED}.</li> + * <li>{@code changeDeletedNode} : {@code Resolution.IGNORED}, .</li> + * <li>{@code deleteChangedNode} : {@code Resolution.IGNORED}, </li> + * <li>{@code deleteDeletedNode} : {@code Resolution.IGNORED}.</li> + * </ul> + */ + +public class PrincipalCacheConflictHandler extends DefaultThreeWayConflictHandler { + + private static final Logger LOG = LoggerFactory.getLogger(PrincipalCacheConflictHandler.class); + + protected static final String REP_EXPIRATION = "rep:expiration"; + protected static final String REP_CACHE = "rep:cache"; + + /** + * Create a new {@code ConflictHandler} which always returns + * {@code resolution}. + * + * @param resolution the resolution to return from all methods of this + * {@code ConflictHandler} instance. + */ + public PrincipalCacheConflictHandler(Resolution resolution) { + super(resolution); + } + public PrincipalCacheConflictHandler() { + this(Resolution.IGNORED); + } + + private Resolution resolveRepExpirationConflict(@NotNull NodeBuilder parent, @NotNull PropertyState ours, @NotNull PropertyState theirs, Review Comment: it would be good if this method would follow common coding style as we use it elsewhere in oak. so, instead of `if ( condition1 && condition2 ){}` it would be `if (condition1 && condition2) {}` either run reformat code or if needed adjust the formatting settings of your ide. i believe we use standard java coding style. if i reformat it, it looks as follows: ``` private Resolution resolveRepExpirationConflict(@NotNull NodeBuilder parent, @NotNull PropertyState ours, @NotNull PropertyState theirs, PropertyState base) { if (REP_EXPIRATION.equals(ours.getName()) && REP_EXPIRATION.equals(theirs.getName())) { PropertyBuilder<Long> merged = PropertyBuilder.scalar(Type.LONG); merged.setName(REP_EXPIRATION); //if base is bigger than ours and theirs, then use base. This should never happens if (base != null && base.getValue(Type.LONG) > ours.getValue(Type.LONG) && base.getValue(Type.LONG) > theirs.getValue(Type.LONG)) { merged.setValue(base.getValue(Type.LONG)); return Resolution.MERGED; } //if ours is bigger than theirs, then use ours //otherwise use theirs if (ours.getValue(Type.LONG) > theirs.getValue(Type.LONG)) { merged.setValue(ours.getValue(Type.LONG)); } else { merged.setValue(theirs.getValue(Type.LONG)); } parent.setProperty(merged.getPropertyState()); LOG.debug("Resolved conflict for property {} our value: {}, merged value: {}", REP_EXPIRATION, ours.getValue(Type.LONG), merged.getValue(0)); return Resolution.MERGED; } return Resolution.IGNORED; } ``` ########## oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/PrincipalCacheConflictHandler.java: ########## @@ -0,0 +1,106 @@ +/* + * 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.user; + +import org.apache.jackrabbit.oak.api.PropertyState; +import org.apache.jackrabbit.oak.api.Type; +import org.apache.jackrabbit.oak.plugins.commit.DefaultThreeWayConflictHandler; +import org.apache.jackrabbit.oak.plugins.memory.PropertyBuilder; +import org.apache.jackrabbit.oak.spi.state.NodeBuilder; +import org.jetbrains.annotations.NotNull; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * The {@code RepMembersConflictHandler} takes care of merging the {@code rep:expiration} property + * during parallel updates. + *<p> + * The conflict handler deals with the following conflicts: + * <ul> + * <li>{@code addExistingProperty} : {@code Resolution.IGNORED}, We should not have add conflints, since the {@code rep:{@code rep:expiration}} node is created with the user</li> + * <li>{@code changeDeletedProperty}: {@code Resolution.IGNORED},</li> + * <li>{@code changeChangedProperty}: {@code Resolution.MERGED}, the properties with higher {@code rep:expiration} get merged</li> + * <li>{@code deleteChangedProperty}: {@code Resolution.IGNORED} .</li> + * <li>{@code deleteDeletedProperty}: {@code Resolution.IGNORED}.</li> + * <li>{@code changeDeletedNode} : {@code Resolution.IGNORED}, .</li> + * <li>{@code deleteChangedNode} : {@code Resolution.IGNORED}, </li> + * <li>{@code deleteDeletedNode} : {@code Resolution.IGNORED}.</li> + * </ul> + */ + +public class PrincipalCacheConflictHandler extends DefaultThreeWayConflictHandler { + + private static final Logger LOG = LoggerFactory.getLogger(PrincipalCacheConflictHandler.class); + + protected static final String REP_EXPIRATION = "rep:expiration"; + protected static final String REP_CACHE = "rep:cache"; + + /** + * Create a new {@code ConflictHandler} which always returns + * {@code resolution}. + * + * @param resolution the resolution to return from all methods of this + * {@code ConflictHandler} instance. + */ + public PrincipalCacheConflictHandler(Resolution resolution) { + super(resolution); + } + public PrincipalCacheConflictHandler() { + this(Resolution.IGNORED); + } + + private Resolution resolveRepExpirationConflict(@NotNull NodeBuilder parent, @NotNull PropertyState ours, @NotNull PropertyState theirs, + PropertyState base) { + if ( REP_EXPIRATION.equals(ours.getName()) && REP_EXPIRATION.equals(theirs.getName()) ){ + + PropertyBuilder<Long> merged = PropertyBuilder.scalar(Type.LONG); + merged.setName(REP_EXPIRATION); + + //if base is bigger than ours and theirs, then use base. This should never happens + if ( base != null && + base.getValue(Type.LONG) > ours.getValue(Type.LONG) && + base.getValue(Type.LONG) > theirs.getValue(Type.LONG) ){ + merged.setValue(base.getValue(Type.LONG)); + return Resolution.MERGED; + } + + //if ours is bigger than theirs, then use ours + //otherwise use theirs + if ( ours.getValue(Type.LONG) > theirs.getValue(Type.LONG) ){ + merged.setValue(ours.getValue(Type.LONG)); + } else { + merged.setValue(theirs.getValue(Type.LONG)); + } + parent.setProperty(merged.getPropertyState()); + LOG.debug("Resolved conflict for property {} our value: {}, merged value: {}", REP_EXPIRATION, ours.getValue(Type.LONG), merged.getValue(0)); + return Resolution.MERGED; + } + return Resolution.IGNORED; + + } + + @Override + public Resolution changeChangedProperty(@NotNull NodeBuilder parent, @NotNull PropertyState ours, @NotNull PropertyState theirs, + @NotNull PropertyState base) { + + return resolveRepExpirationConflict(parent, ours, theirs, base); Review Comment: see initial comment: apart from the expiration also the principal-names could conflict... i don't recall if that was originally part of the reported conflicts, but we may want to consider it. ########## oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserPrincipalProvider.java: ########## @@ -287,7 +287,16 @@ private Set<Principal> getGroupMembership(@NotNull Tree authorizableTree) { // remember the regular groups in case caching is enabled if (doCache) { - cacheGroups(authorizableTree, groupPrincipals); + PrincipalCommitterThread commitThread = UserPrincipalProviderCommitterProvider.getInstance().cacheGroups(authorizableTree, groupPrincipals, expiration, root); + if (commitThread != null) { + //We need to wait the thread to finish, otherwise the session will be closed before the commit is done + try { + commitThread.join(); + } catch (InterruptedException e) { + log.error("Unexpected Error while waiting for commit thread to finish", e); Review Comment: IMHO this error messages should be more precise explaining what the nature of this exception is.... i.e. explaining that it's about writing the principal cache. also, i would recommend to lower the log level to warning. after all there is nothing the oak-consumer did wrong here or could have done differently. an error imo should be actionable, which this problem is not, right? ########## oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/PrincipalCacheConflictHandler.java: ########## @@ -0,0 +1,106 @@ +/* + * 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.user; + +import org.apache.jackrabbit.oak.api.PropertyState; +import org.apache.jackrabbit.oak.api.Type; +import org.apache.jackrabbit.oak.plugins.commit.DefaultThreeWayConflictHandler; +import org.apache.jackrabbit.oak.plugins.memory.PropertyBuilder; +import org.apache.jackrabbit.oak.spi.state.NodeBuilder; +import org.jetbrains.annotations.NotNull; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * The {@code RepMembersConflictHandler} takes care of merging the {@code rep:expiration} property + * during parallel updates. + *<p> + * The conflict handler deals with the following conflicts: + * <ul> + * <li>{@code addExistingProperty} : {@code Resolution.IGNORED}, We should not have add conflints, since the {@code rep:{@code rep:expiration}} node is created with the user</li> + * <li>{@code changeDeletedProperty}: {@code Resolution.IGNORED},</li> + * <li>{@code changeChangedProperty}: {@code Resolution.MERGED}, the properties with higher {@code rep:expiration} get merged</li> + * <li>{@code deleteChangedProperty}: {@code Resolution.IGNORED} .</li> + * <li>{@code deleteDeletedProperty}: {@code Resolution.IGNORED}.</li> + * <li>{@code changeDeletedNode} : {@code Resolution.IGNORED}, .</li> + * <li>{@code deleteChangedNode} : {@code Resolution.IGNORED}, </li> + * <li>{@code deleteDeletedNode} : {@code Resolution.IGNORED}.</li> + * </ul> + */ + +public class PrincipalCacheConflictHandler extends DefaultThreeWayConflictHandler { + + private static final Logger LOG = LoggerFactory.getLogger(PrincipalCacheConflictHandler.class); + + protected static final String REP_EXPIRATION = "rep:expiration"; + protected static final String REP_CACHE = "rep:cache"; + + /** + * Create a new {@code ConflictHandler} which always returns + * {@code resolution}. + * + * @param resolution the resolution to return from all methods of this + * {@code ConflictHandler} instance. + */ + public PrincipalCacheConflictHandler(Resolution resolution) { + super(resolution); + } + public PrincipalCacheConflictHandler() { + this(Resolution.IGNORED); + } + + private Resolution resolveRepExpirationConflict(@NotNull NodeBuilder parent, @NotNull PropertyState ours, @NotNull PropertyState theirs, + PropertyState base) { + if ( REP_EXPIRATION.equals(ours.getName()) && REP_EXPIRATION.equals(theirs.getName()) ){ + + PropertyBuilder<Long> merged = PropertyBuilder.scalar(Type.LONG); + merged.setName(REP_EXPIRATION); + + //if base is bigger than ours and theirs, then use base. This should never happens Review Comment: if it should never happen, should we log something as this is an anomality? ########## oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/PrincipalCacheConflictHandler.java: ########## @@ -0,0 +1,106 @@ +/* + * 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.user; + +import org.apache.jackrabbit.oak.api.PropertyState; +import org.apache.jackrabbit.oak.api.Type; +import org.apache.jackrabbit.oak.plugins.commit.DefaultThreeWayConflictHandler; +import org.apache.jackrabbit.oak.plugins.memory.PropertyBuilder; +import org.apache.jackrabbit.oak.spi.state.NodeBuilder; +import org.jetbrains.annotations.NotNull; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * The {@code RepMembersConflictHandler} takes care of merging the {@code rep:expiration} property Review Comment: i wonder if it might be needed to also cover conflicts with the rep:principalsNames property? ########## oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserPrincipalProviderCommitterProvider.java: ########## @@ -0,0 +1,59 @@ +/* + * 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.user; + +import org.apache.jackrabbit.oak.api.Root; +import org.apache.jackrabbit.oak.api.Tree; +import org.jetbrains.annotations.NotNull; + +import java.security.Principal; +import java.util.HashMap; +import java.util.Set; + +public class UserPrincipalProviderCommitterProvider { + + static UserPrincipalProviderCommitterProvider instance = null; Review Comment: this should be private, no? ########## oak-core/src/test/java/org/apache/jackrabbit/oak/AbstractSecurityTest.java: ########## @@ -89,8 +89,8 @@ public abstract class AbstractSecurityTest { protected Root root; protected QueryEngineSettings querySettings; - private final RootProvider rootProvider = new RootProviderService(); - private final TreeProvider treeProvider = new TreeProviderService(); + protected final RootProvider rootProvider = new RootProviderService(); Review Comment: please use getRootProvider() in the conflict-handler test instead of altering the access modifier ########## oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserPrincipalProviderCommitterProvider.java: ########## @@ -0,0 +1,59 @@ +/* + * 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.user; + +import org.apache.jackrabbit.oak.api.Root; +import org.apache.jackrabbit.oak.api.Tree; +import org.jetbrains.annotations.NotNull; + +import java.security.Principal; +import java.util.HashMap; +import java.util.Set; + +public class UserPrincipalProviderCommitterProvider { + + static UserPrincipalProviderCommitterProvider instance = null; + static HashMap<String, PrincipalCommitterThread> committerThreadMap = new HashMap<>(); Review Comment: nitpicking: should be private static final as it only be used inside the UserPrincipalProviderCommitterProvider on a more serious note: since the map is a singleton i believe it should deal with concurrent modifications as there might be concurrent logins for different users. and another thing: i am not super happy that the map is passed to the thread and the cleanup is done there. at a first glance i thought this was a bug because i didn't see the remove of the entry. so, if possible i would keep the cleanup in the provider. ########## oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserPrincipalProviderCommitterProvider.java: ########## @@ -0,0 +1,59 @@ +/* + * 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.user; + +import org.apache.jackrabbit.oak.api.Root; +import org.apache.jackrabbit.oak.api.Tree; +import org.jetbrains.annotations.NotNull; + +import java.security.Principal; +import java.util.HashMap; +import java.util.Set; + +public class UserPrincipalProviderCommitterProvider { Review Comment: i believe this class doesn't need to be public but can be package protected. naming: the name of the class is a bit hard to process with the duplicate 'Provider' in it. what about something that more precisely describes its function? CacheThreadProvider? this would make it end up being 'close' to `CacheConstants` and `CacheValidatorProvider` in whose proximity it belongs. ########## oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserPrincipalProviderCommitterProvider.java: ########## @@ -0,0 +1,59 @@ +/* + * 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.user; + +import org.apache.jackrabbit.oak.api.Root; +import org.apache.jackrabbit.oak.api.Tree; +import org.jetbrains.annotations.NotNull; + +import java.security.Principal; +import java.util.HashMap; +import java.util.Set; + +public class UserPrincipalProviderCommitterProvider { + + static UserPrincipalProviderCommitterProvider instance = null; + static HashMap<String, PrincipalCommitterThread> committerThreadMap = new HashMap<>(); + + public static UserPrincipalProviderCommitterProvider getInstance() { + if (instance == null) { + instance = new UserPrincipalProviderCommitterProvider(); + } + return instance; + } + + public synchronized PrincipalCommitterThread cacheGroups(@NotNull Tree authorizableNode, @NotNull Set<Principal> groupPrincipals, long expiration, Root root) { + String authorizableNodePath = authorizableNode.getPath(); + if (committerThreadMap.containsKey(authorizableNodePath)) { + // One thread is already committing. return null to inform the caller that doesn't have to wait for the commit to finish + return null; + } else { + PrincipalCommitterThread committerThread = new PrincipalCommitterThread(authorizableNode, groupPrincipals, expiration, root, committerThreadMap); + committerThreadMap.put(authorizableNodePath, committerThread); + committerThread.start(); + return committerThread; + } + } + + public HashMap<String, PrincipalCommitterThread> getCommitterThreadMap() { Review Comment: as far as i can see this method is only used for test verification. i would recommend to drop it and use reflection to assert the state of the map in test-cases..... that leaves no ambiguity and doesn't risk leaking the map (and have it manipulated) by the next dev touching this. ########## oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/PrincipalCommitterThread.java: ########## @@ -0,0 +1,91 @@ +/* + * 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.user; + +import org.apache.jackrabbit.guava.common.base.Joiner; +import org.apache.jackrabbit.guava.common.collect.Iterables; +import org.apache.jackrabbit.oak.api.CommitFailedException; +import org.apache.jackrabbit.oak.api.Root; +import org.apache.jackrabbit.oak.api.Tree; +import org.apache.jackrabbit.oak.commons.LongUtils; +import org.apache.jackrabbit.oak.plugins.tree.TreeUtil; +import org.apache.jackrabbit.util.Text; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import javax.jcr.AccessDeniedException; +import java.security.Principal; +import java.util.HashMap; +import java.util.Set; + +import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.MEMBERSHIP_THRESHOLD; Review Comment: the constant doesn't need to be imported but could simply be moved here as it is not used elsewhere ########## oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserPrincipalProvider.java: ########## @@ -287,7 +287,16 @@ private Set<Principal> getGroupMembership(@NotNull Tree authorizableTree) { // remember the regular groups in case caching is enabled if (doCache) { - cacheGroups(authorizableTree, groupPrincipals); + PrincipalCommitterThread commitThread = UserPrincipalProviderCommitterProvider.getInstance().cacheGroups(authorizableTree, groupPrincipals, expiration, root); Review Comment: the commitThread could just be a Thread object. it the userprincipalprovider can be ignorant about the implementation, no? ########## oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/PrincipalCommitterThread.java: ########## @@ -0,0 +1,91 @@ +/* + * 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.user; + +import org.apache.jackrabbit.guava.common.base.Joiner; +import org.apache.jackrabbit.guava.common.collect.Iterables; +import org.apache.jackrabbit.oak.api.CommitFailedException; +import org.apache.jackrabbit.oak.api.Root; +import org.apache.jackrabbit.oak.api.Tree; +import org.apache.jackrabbit.oak.commons.LongUtils; +import org.apache.jackrabbit.oak.plugins.tree.TreeUtil; +import org.apache.jackrabbit.util.Text; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import javax.jcr.AccessDeniedException; +import java.security.Principal; +import java.util.HashMap; +import java.util.Set; + +import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.MEMBERSHIP_THRESHOLD; + +public class PrincipalCommitterThread extends Thread { + Tree authorizableNode; + Set<Principal> groupPrincipals; + HashMap committerThreadMap; + long expiration; + Root root; + + private static final Logger log = LoggerFactory.getLogger(PrincipalCommitterThread.class); + + public PrincipalCommitterThread(Tree authorizableNode, Set<Principal> groupPrincipals, long expiration, Root root, HashMap committerThreadMap) { Review Comment: - constructor should not be public - params should be annotation with `@NotNull` (except the expiration) ########## oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/PrincipalCommitterThread.java: ########## @@ -0,0 +1,91 @@ +/* + * 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.user; + +import org.apache.jackrabbit.guava.common.base.Joiner; +import org.apache.jackrabbit.guava.common.collect.Iterables; +import org.apache.jackrabbit.oak.api.CommitFailedException; +import org.apache.jackrabbit.oak.api.Root; +import org.apache.jackrabbit.oak.api.Tree; +import org.apache.jackrabbit.oak.commons.LongUtils; +import org.apache.jackrabbit.oak.plugins.tree.TreeUtil; +import org.apache.jackrabbit.util.Text; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import javax.jcr.AccessDeniedException; +import java.security.Principal; +import java.util.HashMap; +import java.util.Set; + +import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.MEMBERSHIP_THRESHOLD; + +public class PrincipalCommitterThread extends Thread { Review Comment: i would recommend to move this class as an inner class of the UserPrincipalProviderCommitterProvider. nobody outside needs to know it, right? that might also solve the issue with passing around the map and the removal of entries on that map that i complained about in UserPrincipalProviderCommitterProvider. at the very least i must not be a public class. naming: it's not precisely about committing principals but about the cache, right? so i would reconsider the naming (but that would be sort of irrelevant if it were a private inner class) ########## oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/PrincipalCommitterThread.java: ########## @@ -0,0 +1,91 @@ +/* + * 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.user; + +import org.apache.jackrabbit.guava.common.base.Joiner; +import org.apache.jackrabbit.guava.common.collect.Iterables; +import org.apache.jackrabbit.oak.api.CommitFailedException; +import org.apache.jackrabbit.oak.api.Root; +import org.apache.jackrabbit.oak.api.Tree; +import org.apache.jackrabbit.oak.commons.LongUtils; +import org.apache.jackrabbit.oak.plugins.tree.TreeUtil; +import org.apache.jackrabbit.util.Text; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import javax.jcr.AccessDeniedException; +import java.security.Principal; +import java.util.HashMap; +import java.util.Set; + +import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.MEMBERSHIP_THRESHOLD; + +public class PrincipalCommitterThread extends Thread { + Tree authorizableNode; + Set<Principal> groupPrincipals; + HashMap committerThreadMap; + long expiration; + Root root; Review Comment: all instance fields of this class should be `private final` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
