anchela commented on code in PR #1128: URL: https://github.com/apache/jackrabbit-oak/pull/1128#discussion_r1371652407
########## oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/GroupMembershipReader.java: ########## @@ -0,0 +1,73 @@ +/* + * 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 java.security.Principal; +import java.util.Iterator; +import java.util.Set; +import java.util.function.BiFunction; +import java.util.function.Function; + +import org.apache.jackrabbit.oak.api.Tree; +import org.apache.jackrabbit.oak.spi.security.user.AuthorizableType; +import org.apache.jackrabbit.oak.spi.security.user.util.UserUtil; +import org.jetbrains.annotations.NotNull; + +import static org.apache.jackrabbit.guava.common.base.Preconditions.checkNotNull; + +/** + * <code>GroupMembershipReader</code>... + */ +class GroupMembershipReader { + + private final MembershipProvider membershipProvider; + + protected final GroupPrincipalFactory groupPrincipalFactory; + + GroupMembershipReader(@NotNull MembershipProvider membershipProvider, + @NotNull GroupPrincipalFactory groupPrincipalFactory) { + this.membershipProvider = checkNotNull(membershipProvider); + this.groupPrincipalFactory = checkNotNull(groupPrincipalFactory); + } + + void getMembership(@NotNull Tree authorizable, + @NotNull Set<Principal> groupPrincipals) { + loadGroupPrincipals(authorizable, groupPrincipals); + } + + protected final void loadGroupPrincipals(@NotNull Tree authorizableTree, Review Comment: same here. why does this need to be 'protected'? ########## oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserPrincipalProvider.java: ########## @@ -209,6 +203,21 @@ public Iterator<? extends Principal> findPrincipals(int searchType) { } //------------------------------------------------------------< private >--- + + GroupPrincipalFactory createGroupPrincipalFactory() { Review Comment: the return value should be annotated with '@NotNull' and the methods should be private. ########## oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/GroupMembershipReader.java: ########## @@ -0,0 +1,73 @@ +/* + * 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 java.security.Principal; +import java.util.Iterator; +import java.util.Set; +import java.util.function.BiFunction; +import java.util.function.Function; + +import org.apache.jackrabbit.oak.api.Tree; +import org.apache.jackrabbit.oak.spi.security.user.AuthorizableType; +import org.apache.jackrabbit.oak.spi.security.user.util.UserUtil; +import org.jetbrains.annotations.NotNull; + +import static org.apache.jackrabbit.guava.common.base.Preconditions.checkNotNull; + +/** + * <code>GroupMembershipReader</code>... + */ +class GroupMembershipReader { + + private final MembershipProvider membershipProvider; + + protected final GroupPrincipalFactory groupPrincipalFactory; + + GroupMembershipReader(@NotNull MembershipProvider membershipProvider, + @NotNull GroupPrincipalFactory groupPrincipalFactory) { + this.membershipProvider = checkNotNull(membershipProvider); + this.groupPrincipalFactory = checkNotNull(groupPrincipalFactory); + } + + void getMembership(@NotNull Tree authorizable, + @NotNull Set<Principal> groupPrincipals) { + loadGroupPrincipals(authorizable, groupPrincipals); + } + + protected final void loadGroupPrincipals(@NotNull Tree authorizableTree, + @NotNull Set<Principal> groupPrincipals) { + Iterator<Tree> groupTrees = membershipProvider.getMembership(authorizableTree, true); + while (groupTrees.hasNext()) { + Tree groupTree = groupTrees.next(); + if (UserUtil.isType(groupTree, AuthorizableType.GROUP)) { + Principal gr = groupPrincipalFactory.create(groupTree); + if (gr != null) { + groupPrincipals.add(gr); + } + } + } + } + + interface GroupPrincipalFactory { + + Principal create(@NotNull Tree authorizable); Review Comment: return value should be annotated with NotNull or Nullable ########## oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/CachedGroupMembershipReader.java: ########## @@ -0,0 +1,209 @@ +/* + * 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 java.security.Principal; +import java.util.LinkedHashMap; +import java.util.Map; +import java.util.Objects; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.function.BiConsumer; + +import javax.jcr.AccessDeniedException; + +import org.apache.jackrabbit.guava.common.base.Joiner; +import org.apache.jackrabbit.guava.common.base.Strings; +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.oak.spi.security.user.AuthorizableType; +import org.apache.jackrabbit.oak.spi.security.user.UserConfiguration; +import org.apache.jackrabbit.oak.spi.security.user.util.UserUtil; +import org.apache.jackrabbit.util.Text; +import org.jetbrains.annotations.NotNull; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import static org.apache.jackrabbit.guava.common.base.Preconditions.checkNotNull; +import static org.apache.jackrabbit.oak.security.user.CacheConstants.REP_GROUP_PRINCIPAL_NAMES; +import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.EXPIRATION_NO_CACHE; +import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.NO_STALE_CACHE; +import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.PARAM_CACHE_EXPIRATION; +import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.PARAM_CACHE_MAX_STALE; + +/** + * <code>CachedGroupMembershipReader</code>... + */ +class CachedGroupMembershipReader extends GroupMembershipReader { + + private static final Logger LOG = LoggerFactory.getLogger(CachedGroupMembershipReader.class); + + private static final long MEMBERSHIP_THRESHOLD = 0; + + /** + * Keep track of cache updates for 100 most recent authorizables. + */ + private static final int MAX_CACHE_TRACKING_ENTRIES = 100; + + private static final Map<UserConfiguration, Map<String, Long>> CACHE_UPDATES = new ConcurrentHashMap<>(); + + private final UserConfiguration config; + + private final Root root; + + private final long expiration; + private final long maxStale; + + CachedGroupMembershipReader(@NotNull MembershipProvider membershipProvider, + @NotNull GroupPrincipalFactory groupPrincipalFactory, + @NotNull UserConfiguration config, + @NotNull Root root) { + super(membershipProvider, groupPrincipalFactory); + this.config = checkNotNull(config); + this.root = root; + this.expiration = config.getParameters().getConfigValue(PARAM_CACHE_EXPIRATION, EXPIRATION_NO_CACHE); + this.maxStale = config.getParameters().getConfigValue(PARAM_CACHE_MAX_STALE, NO_STALE_CACHE); + } + + @Override + void getMembership(@NotNull Tree authorizable, + @NotNull Set<Principal> groupPrincipals) { + // membership cache only implemented on user + if (UserUtil.isType(authorizable, AuthorizableType.USER)) { + readGroupsFromCache(authorizable, groupPrincipals, this::loadGroupPrincipals); + } else { + loadGroupPrincipals(authorizable, groupPrincipals); + } + } + + private void readGroupsFromCache(@NotNull Tree authorizableNode, + @NotNull Set<Principal> groups, + @NotNull BiConsumer<Tree, Set<Principal>> loader) { + long expirationTime = 0; + String authorizablePath = authorizableNode.getPath(); + Tree principalCache = authorizableNode.getChild(CacheConstants.REP_CACHE); + if (principalCache.exists()) { + expirationTime = TreeUtil.getLong(principalCache, CacheConstants.REP_EXPIRATION, EXPIRATION_NO_CACHE); + } + long now = System.currentTimeMillis(); + if (expirationTime > EXPIRATION_NO_CACHE && now < expirationTime) { + serveGroupsFromCache(authorizablePath, principalCache, groups); + return; + } + + // need to load or serve stale if allowed + boolean mayServeStale = now - expirationTime < maxStale; + boolean updateCache; + Map<String, Long> updates = getCacheUpdateMap(); + synchronized (updates) { + // anyone else updating that entry already? Review Comment: instead of asking a question, i would find the comment a lot more helpful if it would explain. "test if the cache for the given user is already being updated. this is achieved by ....." (please fill in .... to make it easily understandable for anyone looking at this code. at a first glance that was immediately obvious to me and how the exp time play a role here) ########## oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserPrincipalProvider.java: ########## @@ -209,6 +203,21 @@ public Iterator<? extends Principal> findPrincipals(int searchType) { } //------------------------------------------------------------< private >--- + + GroupPrincipalFactory createGroupPrincipalFactory() { Review Comment: return value should be annotated with @NotNull ########## oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/CachedGroupMembershipReader.java: ########## @@ -0,0 +1,209 @@ +/* + * 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 java.security.Principal; +import java.util.LinkedHashMap; +import java.util.Map; +import java.util.Objects; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.function.BiConsumer; + +import javax.jcr.AccessDeniedException; + +import org.apache.jackrabbit.guava.common.base.Joiner; +import org.apache.jackrabbit.guava.common.base.Strings; +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.oak.spi.security.user.AuthorizableType; +import org.apache.jackrabbit.oak.spi.security.user.UserConfiguration; +import org.apache.jackrabbit.oak.spi.security.user.util.UserUtil; +import org.apache.jackrabbit.util.Text; +import org.jetbrains.annotations.NotNull; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import static org.apache.jackrabbit.guava.common.base.Preconditions.checkNotNull; +import static org.apache.jackrabbit.oak.security.user.CacheConstants.REP_GROUP_PRINCIPAL_NAMES; +import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.EXPIRATION_NO_CACHE; +import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.NO_STALE_CACHE; +import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.PARAM_CACHE_EXPIRATION; +import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.PARAM_CACHE_MAX_STALE; + +/** + * <code>CachedGroupMembershipReader</code>... + */ +class CachedGroupMembershipReader extends GroupMembershipReader { + + private static final Logger LOG = LoggerFactory.getLogger(CachedGroupMembershipReader.class); + + private static final long MEMBERSHIP_THRESHOLD = 0; + + /** + * Keep track of cache updates for 100 most recent authorizables. + */ + private static final int MAX_CACHE_TRACKING_ENTRIES = 100; + + private static final Map<UserConfiguration, Map<String, Long>> CACHE_UPDATES = new ConcurrentHashMap<>(); + + private final UserConfiguration config; + + private final Root root; + + private final long expiration; + private final long maxStale; + + CachedGroupMembershipReader(@NotNull MembershipProvider membershipProvider, + @NotNull GroupPrincipalFactory groupPrincipalFactory, + @NotNull UserConfiguration config, + @NotNull Root root) { + super(membershipProvider, groupPrincipalFactory); + this.config = checkNotNull(config); + this.root = root; + this.expiration = config.getParameters().getConfigValue(PARAM_CACHE_EXPIRATION, EXPIRATION_NO_CACHE); + this.maxStale = config.getParameters().getConfigValue(PARAM_CACHE_MAX_STALE, NO_STALE_CACHE); + } + + @Override + void getMembership(@NotNull Tree authorizable, + @NotNull Set<Principal> groupPrincipals) { + // membership cache only implemented on user + if (UserUtil.isType(authorizable, AuthorizableType.USER)) { + readGroupsFromCache(authorizable, groupPrincipals, this::loadGroupPrincipals); + } else { + loadGroupPrincipals(authorizable, groupPrincipals); + } + } + + private void readGroupsFromCache(@NotNull Tree authorizableNode, + @NotNull Set<Principal> groups, + @NotNull BiConsumer<Tree, Set<Principal>> loader) { + long expirationTime = 0; + String authorizablePath = authorizableNode.getPath(); + Tree principalCache = authorizableNode.getChild(CacheConstants.REP_CACHE); + if (principalCache.exists()) { + expirationTime = TreeUtil.getLong(principalCache, CacheConstants.REP_EXPIRATION, EXPIRATION_NO_CACHE); + } + long now = System.currentTimeMillis(); + if (expirationTime > EXPIRATION_NO_CACHE && now < expirationTime) { + serveGroupsFromCache(authorizablePath, principalCache, groups); + return; + } + + // need to load or serve stale if allowed + boolean mayServeStale = now - expirationTime < maxStale; + boolean updateCache; + Map<String, Long> updates = getCacheUpdateMap(); + synchronized (updates) { + // anyone else updating that entry already? + Long exp = updates.get(authorizablePath); + updateCache = exp == null || expirationTime > exp; + if (updateCache) { + // we do it + updates.put(authorizablePath, expirationTime); + } + } + + if (!updateCache && mayServeStale) { + LOG.debug("Another thread is updating the cache and we may serve stale"); + serveGroupsFromCache(authorizablePath, principalCache, groups); + return; + } + + if (updateCache) { + boolean cacheSuccess = false; + try { + loader.accept(authorizableNode, groups); + cacheGroups(authorizableNode, groups); + cacheSuccess = true; + } catch (AccessDeniedException | CommitFailedException e) { + LOG.debug("Failed to cache group membership: {}", e.getMessage()); Review Comment: i think i would log that as a warning. not just on debug level. ########## oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/CachedGroupMembershipReader.java: ########## @@ -0,0 +1,209 @@ +/* + * 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 java.security.Principal; +import java.util.LinkedHashMap; +import java.util.Map; +import java.util.Objects; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.function.BiConsumer; + +import javax.jcr.AccessDeniedException; + +import org.apache.jackrabbit.guava.common.base.Joiner; +import org.apache.jackrabbit.guava.common.base.Strings; +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.oak.spi.security.user.AuthorizableType; +import org.apache.jackrabbit.oak.spi.security.user.UserConfiguration; +import org.apache.jackrabbit.oak.spi.security.user.util.UserUtil; +import org.apache.jackrabbit.util.Text; +import org.jetbrains.annotations.NotNull; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import static org.apache.jackrabbit.guava.common.base.Preconditions.checkNotNull; +import static org.apache.jackrabbit.oak.security.user.CacheConstants.REP_GROUP_PRINCIPAL_NAMES; +import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.EXPIRATION_NO_CACHE; +import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.NO_STALE_CACHE; +import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.PARAM_CACHE_EXPIRATION; +import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.PARAM_CACHE_MAX_STALE; + +/** + * <code>CachedGroupMembershipReader</code>... + */ +class CachedGroupMembershipReader extends GroupMembershipReader { + + private static final Logger LOG = LoggerFactory.getLogger(CachedGroupMembershipReader.class); + + private static final long MEMBERSHIP_THRESHOLD = 0; + + /** + * Keep track of cache updates for 100 most recent authorizables. + */ + private static final int MAX_CACHE_TRACKING_ENTRIES = 100; + + private static final Map<UserConfiguration, Map<String, Long>> CACHE_UPDATES = new ConcurrentHashMap<>(); + + private final UserConfiguration config; + + private final Root root; + + private final long expiration; + private final long maxStale; + + CachedGroupMembershipReader(@NotNull MembershipProvider membershipProvider, + @NotNull GroupPrincipalFactory groupPrincipalFactory, + @NotNull UserConfiguration config, + @NotNull Root root) { + super(membershipProvider, groupPrincipalFactory); + this.config = checkNotNull(config); + this.root = root; + this.expiration = config.getParameters().getConfigValue(PARAM_CACHE_EXPIRATION, EXPIRATION_NO_CACHE); + this.maxStale = config.getParameters().getConfigValue(PARAM_CACHE_MAX_STALE, NO_STALE_CACHE); + } + + @Override + void getMembership(@NotNull Tree authorizable, + @NotNull Set<Principal> groupPrincipals) { + // membership cache only implemented on user + if (UserUtil.isType(authorizable, AuthorizableType.USER)) { + readGroupsFromCache(authorizable, groupPrincipals, this::loadGroupPrincipals); + } else { + loadGroupPrincipals(authorizable, groupPrincipals); + } + } + + private void readGroupsFromCache(@NotNull Tree authorizableNode, + @NotNull Set<Principal> groups, + @NotNull BiConsumer<Tree, Set<Principal>> loader) { + long expirationTime = 0; + String authorizablePath = authorizableNode.getPath(); + Tree principalCache = authorizableNode.getChild(CacheConstants.REP_CACHE); + if (principalCache.exists()) { + expirationTime = TreeUtil.getLong(principalCache, CacheConstants.REP_EXPIRATION, EXPIRATION_NO_CACHE); + } + long now = System.currentTimeMillis(); + if (expirationTime > EXPIRATION_NO_CACHE && now < expirationTime) { + serveGroupsFromCache(authorizablePath, principalCache, groups); + return; + } + + // need to load or serve stale if allowed + boolean mayServeStale = now - expirationTime < maxStale; + boolean updateCache; + Map<String, Long> updates = getCacheUpdateMap(); + synchronized (updates) { + // anyone else updating that entry already? + Long exp = updates.get(authorizablePath); + updateCache = exp == null || expirationTime > exp; + if (updateCache) { + // we do it + updates.put(authorizablePath, expirationTime); + } + } + + if (!updateCache && mayServeStale) { + LOG.debug("Another thread is updating the cache and we may serve stale"); Review Comment: see above about the usage of 'may'. i would change the phrasing of this log output to make it meaningful for someone that is not familiar with the inner working of the cache. so explicitly stating that potentially outdated values are returned. ########## oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/CachedGroupMembershipReader.java: ########## @@ -0,0 +1,209 @@ +/* + * 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 java.security.Principal; +import java.util.LinkedHashMap; +import java.util.Map; +import java.util.Objects; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.function.BiConsumer; + +import javax.jcr.AccessDeniedException; + +import org.apache.jackrabbit.guava.common.base.Joiner; +import org.apache.jackrabbit.guava.common.base.Strings; +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.oak.spi.security.user.AuthorizableType; +import org.apache.jackrabbit.oak.spi.security.user.UserConfiguration; +import org.apache.jackrabbit.oak.spi.security.user.util.UserUtil; +import org.apache.jackrabbit.util.Text; +import org.jetbrains.annotations.NotNull; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import static org.apache.jackrabbit.guava.common.base.Preconditions.checkNotNull; +import static org.apache.jackrabbit.oak.security.user.CacheConstants.REP_GROUP_PRINCIPAL_NAMES; +import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.EXPIRATION_NO_CACHE; +import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.NO_STALE_CACHE; +import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.PARAM_CACHE_EXPIRATION; +import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.PARAM_CACHE_MAX_STALE; + +/** + * <code>CachedGroupMembershipReader</code>... + */ +class CachedGroupMembershipReader extends GroupMembershipReader { + + private static final Logger LOG = LoggerFactory.getLogger(CachedGroupMembershipReader.class); + + private static final long MEMBERSHIP_THRESHOLD = 0; + + /** + * Keep track of cache updates for 100 most recent authorizables. + */ + private static final int MAX_CACHE_TRACKING_ENTRIES = 100; + + private static final Map<UserConfiguration, Map<String, Long>> CACHE_UPDATES = new ConcurrentHashMap<>(); + + private final UserConfiguration config; + + private final Root root; + + private final long expiration; + private final long maxStale; + + CachedGroupMembershipReader(@NotNull MembershipProvider membershipProvider, + @NotNull GroupPrincipalFactory groupPrincipalFactory, + @NotNull UserConfiguration config, + @NotNull Root root) { + super(membershipProvider, groupPrincipalFactory); + this.config = checkNotNull(config); + this.root = root; + this.expiration = config.getParameters().getConfigValue(PARAM_CACHE_EXPIRATION, EXPIRATION_NO_CACHE); + this.maxStale = config.getParameters().getConfigValue(PARAM_CACHE_MAX_STALE, NO_STALE_CACHE); + } + + @Override + void getMembership(@NotNull Tree authorizable, + @NotNull Set<Principal> groupPrincipals) { + // membership cache only implemented on user + if (UserUtil.isType(authorizable, AuthorizableType.USER)) { + readGroupsFromCache(authorizable, groupPrincipals, this::loadGroupPrincipals); + } else { + loadGroupPrincipals(authorizable, groupPrincipals); + } + } + + private void readGroupsFromCache(@NotNull Tree authorizableNode, + @NotNull Set<Principal> groups, + @NotNull BiConsumer<Tree, Set<Principal>> loader) { + long expirationTime = 0; + String authorizablePath = authorizableNode.getPath(); + Tree principalCache = authorizableNode.getChild(CacheConstants.REP_CACHE); + if (principalCache.exists()) { + expirationTime = TreeUtil.getLong(principalCache, CacheConstants.REP_EXPIRATION, EXPIRATION_NO_CACHE); + } + long now = System.currentTimeMillis(); + if (expirationTime > EXPIRATION_NO_CACHE && now < expirationTime) { + serveGroupsFromCache(authorizablePath, principalCache, groups); + return; + } + + // need to load or serve stale if allowed + boolean mayServeStale = now - expirationTime < maxStale; + boolean updateCache; + Map<String, Long> updates = getCacheUpdateMap(); + synchronized (updates) { + // anyone else updating that entry already? + Long exp = updates.get(authorizablePath); + updateCache = exp == null || expirationTime > exp; + if (updateCache) { + // we do it Review Comment: we do what? please elaborate ########## oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/GroupMembershipReader.java: ########## @@ -0,0 +1,73 @@ +/* + * 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 java.security.Principal; +import java.util.Iterator; +import java.util.Set; +import java.util.function.BiFunction; +import java.util.function.Function; + +import org.apache.jackrabbit.oak.api.Tree; +import org.apache.jackrabbit.oak.spi.security.user.AuthorizableType; +import org.apache.jackrabbit.oak.spi.security.user.util.UserUtil; +import org.jetbrains.annotations.NotNull; + +import static org.apache.jackrabbit.guava.common.base.Preconditions.checkNotNull; + +/** + * <code>GroupMembershipReader</code>... + */ +class GroupMembershipReader { + + private final MembershipProvider membershipProvider; + + protected final GroupPrincipalFactory groupPrincipalFactory; + + GroupMembershipReader(@NotNull MembershipProvider membershipProvider, + @NotNull GroupPrincipalFactory groupPrincipalFactory) { + this.membershipProvider = checkNotNull(membershipProvider); + this.groupPrincipalFactory = checkNotNull(groupPrincipalFactory); + } + + void getMembership(@NotNull Tree authorizable, + @NotNull Set<Principal> groupPrincipals) { + loadGroupPrincipals(authorizable, groupPrincipals); + } + + protected final void loadGroupPrincipals(@NotNull Tree authorizableTree, + @NotNull Set<Principal> groupPrincipals) { + Iterator<Tree> groupTrees = membershipProvider.getMembership(authorizableTree, true); + while (groupTrees.hasNext()) { + Tree groupTree = groupTrees.next(); + if (UserUtil.isType(groupTree, AuthorizableType.GROUP)) { + Principal gr = groupPrincipalFactory.create(groupTree); + if (gr != null) { + groupPrincipals.add(gr); + } + } + } + } + + interface GroupPrincipalFactory { + + Principal create(@NotNull Tree authorizable); + + Principal create(@NotNull String principalName); Review Comment: return value should be annotated with NotNull or Nullable ########## oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/CachedGroupMembershipReader.java: ########## @@ -0,0 +1,209 @@ +/* + * 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 java.security.Principal; +import java.util.LinkedHashMap; +import java.util.Map; +import java.util.Objects; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.function.BiConsumer; + +import javax.jcr.AccessDeniedException; + +import org.apache.jackrabbit.guava.common.base.Joiner; +import org.apache.jackrabbit.guava.common.base.Strings; +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.oak.spi.security.user.AuthorizableType; +import org.apache.jackrabbit.oak.spi.security.user.UserConfiguration; +import org.apache.jackrabbit.oak.spi.security.user.util.UserUtil; +import org.apache.jackrabbit.util.Text; +import org.jetbrains.annotations.NotNull; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import static org.apache.jackrabbit.guava.common.base.Preconditions.checkNotNull; +import static org.apache.jackrabbit.oak.security.user.CacheConstants.REP_GROUP_PRINCIPAL_NAMES; +import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.EXPIRATION_NO_CACHE; +import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.NO_STALE_CACHE; +import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.PARAM_CACHE_EXPIRATION; +import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.PARAM_CACHE_MAX_STALE; + +/** + * <code>CachedGroupMembershipReader</code>... Review Comment: either add proper javadoc or drop it. and again.... rather use {@code} and <code> ########## oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/CachedGroupMembershipReader.java: ########## @@ -0,0 +1,209 @@ +/* + * 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 java.security.Principal; +import java.util.LinkedHashMap; +import java.util.Map; +import java.util.Objects; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.function.BiConsumer; + +import javax.jcr.AccessDeniedException; + +import org.apache.jackrabbit.guava.common.base.Joiner; +import org.apache.jackrabbit.guava.common.base.Strings; +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.oak.spi.security.user.AuthorizableType; +import org.apache.jackrabbit.oak.spi.security.user.UserConfiguration; +import org.apache.jackrabbit.oak.spi.security.user.util.UserUtil; +import org.apache.jackrabbit.util.Text; +import org.jetbrains.annotations.NotNull; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import static org.apache.jackrabbit.guava.common.base.Preconditions.checkNotNull; +import static org.apache.jackrabbit.oak.security.user.CacheConstants.REP_GROUP_PRINCIPAL_NAMES; +import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.EXPIRATION_NO_CACHE; +import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.NO_STALE_CACHE; +import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.PARAM_CACHE_EXPIRATION; +import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.PARAM_CACHE_MAX_STALE; + +/** + * <code>CachedGroupMembershipReader</code>... + */ +class CachedGroupMembershipReader extends GroupMembershipReader { + + private static final Logger LOG = LoggerFactory.getLogger(CachedGroupMembershipReader.class); + + private static final long MEMBERSHIP_THRESHOLD = 0; + + /** + * Keep track of cache updates for 100 most recent authorizables. + */ + private static final int MAX_CACHE_TRACKING_ENTRIES = 100; + + private static final Map<UserConfiguration, Map<String, Long>> CACHE_UPDATES = new ConcurrentHashMap<>(); + + private final UserConfiguration config; + + private final Root root; + + private final long expiration; + private final long maxStale; + + CachedGroupMembershipReader(@NotNull MembershipProvider membershipProvider, + @NotNull GroupPrincipalFactory groupPrincipalFactory, + @NotNull UserConfiguration config, + @NotNull Root root) { + super(membershipProvider, groupPrincipalFactory); + this.config = checkNotNull(config); + this.root = root; + this.expiration = config.getParameters().getConfigValue(PARAM_CACHE_EXPIRATION, EXPIRATION_NO_CACHE); + this.maxStale = config.getParameters().getConfigValue(PARAM_CACHE_MAX_STALE, NO_STALE_CACHE); + } + + @Override + void getMembership(@NotNull Tree authorizable, + @NotNull Set<Principal> groupPrincipals) { + // membership cache only implemented on user + if (UserUtil.isType(authorizable, AuthorizableType.USER)) { + readGroupsFromCache(authorizable, groupPrincipals, this::loadGroupPrincipals); + } else { + loadGroupPrincipals(authorizable, groupPrincipals); + } + } + + private void readGroupsFromCache(@NotNull Tree authorizableNode, + @NotNull Set<Principal> groups, + @NotNull BiConsumer<Tree, Set<Principal>> loader) { + long expirationTime = 0; + String authorizablePath = authorizableNode.getPath(); + Tree principalCache = authorizableNode.getChild(CacheConstants.REP_CACHE); + if (principalCache.exists()) { + expirationTime = TreeUtil.getLong(principalCache, CacheConstants.REP_EXPIRATION, EXPIRATION_NO_CACHE); + } + long now = System.currentTimeMillis(); + if (expirationTime > EXPIRATION_NO_CACHE && now < expirationTime) { + serveGroupsFromCache(authorizablePath, principalCache, groups); + return; + } + + // need to load or serve stale if allowed + boolean mayServeStale = now - expirationTime < maxStale; + boolean updateCache; + Map<String, Long> updates = getCacheUpdateMap(); + synchronized (updates) { + // anyone else updating that entry already? + Long exp = updates.get(authorizablePath); + updateCache = exp == null || expirationTime > exp; Review Comment: i would find it easier to read with () around the or-ing expression: updateCache = (exp == null || expirationTime > exp). to have a visual distinction between the assignment and the subsequent '==' ########## oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/GroupMembershipReader.java: ########## @@ -0,0 +1,73 @@ +/* + * 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 java.security.Principal; +import java.util.Iterator; +import java.util.Set; +import java.util.function.BiFunction; +import java.util.function.Function; + +import org.apache.jackrabbit.oak.api.Tree; +import org.apache.jackrabbit.oak.spi.security.user.AuthorizableType; +import org.apache.jackrabbit.oak.spi.security.user.util.UserUtil; +import org.jetbrains.annotations.NotNull; + +import static org.apache.jackrabbit.guava.common.base.Preconditions.checkNotNull; + +/** + * <code>GroupMembershipReader</code>... Review Comment: this deserves a bit of javadoc. if we believe it's not neeeded drop it altogether. apart from that: modern style javadoc uses {@code} instead of <code>... my IDE even complains about it :) ########## oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/CachedGroupMembershipReader.java: ########## @@ -0,0 +1,209 @@ +/* + * 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 java.security.Principal; +import java.util.LinkedHashMap; +import java.util.Map; +import java.util.Objects; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.function.BiConsumer; + +import javax.jcr.AccessDeniedException; + +import org.apache.jackrabbit.guava.common.base.Joiner; +import org.apache.jackrabbit.guava.common.base.Strings; +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.oak.spi.security.user.AuthorizableType; +import org.apache.jackrabbit.oak.spi.security.user.UserConfiguration; +import org.apache.jackrabbit.oak.spi.security.user.util.UserUtil; +import org.apache.jackrabbit.util.Text; +import org.jetbrains.annotations.NotNull; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import static org.apache.jackrabbit.guava.common.base.Preconditions.checkNotNull; +import static org.apache.jackrabbit.oak.security.user.CacheConstants.REP_GROUP_PRINCIPAL_NAMES; +import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.EXPIRATION_NO_CACHE; +import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.NO_STALE_CACHE; +import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.PARAM_CACHE_EXPIRATION; +import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.PARAM_CACHE_MAX_STALE; + +/** + * <code>CachedGroupMembershipReader</code>... + */ +class CachedGroupMembershipReader extends GroupMembershipReader { + + private static final Logger LOG = LoggerFactory.getLogger(CachedGroupMembershipReader.class); + + private static final long MEMBERSHIP_THRESHOLD = 0; + + /** + * Keep track of cache updates for 100 most recent authorizables. + */ + private static final int MAX_CACHE_TRACKING_ENTRIES = 100; + + private static final Map<UserConfiguration, Map<String, Long>> CACHE_UPDATES = new ConcurrentHashMap<>(); + + private final UserConfiguration config; + + private final Root root; + + private final long expiration; + private final long maxStale; + + CachedGroupMembershipReader(@NotNull MembershipProvider membershipProvider, + @NotNull GroupPrincipalFactory groupPrincipalFactory, + @NotNull UserConfiguration config, + @NotNull Root root) { + super(membershipProvider, groupPrincipalFactory); + this.config = checkNotNull(config); + this.root = root; + this.expiration = config.getParameters().getConfigValue(PARAM_CACHE_EXPIRATION, EXPIRATION_NO_CACHE); + this.maxStale = config.getParameters().getConfigValue(PARAM_CACHE_MAX_STALE, NO_STALE_CACHE); + } + + @Override + void getMembership(@NotNull Tree authorizable, + @NotNull Set<Principal> groupPrincipals) { + // membership cache only implemented on user + if (UserUtil.isType(authorizable, AuthorizableType.USER)) { + readGroupsFromCache(authorizable, groupPrincipals, this::loadGroupPrincipals); + } else { + loadGroupPrincipals(authorizable, groupPrincipals); + } + } + + private void readGroupsFromCache(@NotNull Tree authorizableNode, + @NotNull Set<Principal> groups, + @NotNull BiConsumer<Tree, Set<Principal>> loader) { + long expirationTime = 0; + String authorizablePath = authorizableNode.getPath(); + Tree principalCache = authorizableNode.getChild(CacheConstants.REP_CACHE); + if (principalCache.exists()) { + expirationTime = TreeUtil.getLong(principalCache, CacheConstants.REP_EXPIRATION, EXPIRATION_NO_CACHE); + } + long now = System.currentTimeMillis(); + if (expirationTime > EXPIRATION_NO_CACHE && now < expirationTime) { + serveGroupsFromCache(authorizablePath, principalCache, groups); + return; + } + + // need to load or serve stale if allowed + boolean mayServeStale = now - expirationTime < maxStale; Review Comment: calculation of mayServeStale can be moved down closer to the if where it is evaluated such that i don't have to scroll up again to check. also: why is it 'may'? if that variable is true and updating the cache was not possible, we do return a stale cache, no? so may sounds wrong to me. ########## oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/GroupMembershipReader.java: ########## @@ -0,0 +1,73 @@ +/* + * 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 java.security.Principal; +import java.util.Iterator; +import java.util.Set; +import java.util.function.BiFunction; +import java.util.function.Function; + +import org.apache.jackrabbit.oak.api.Tree; +import org.apache.jackrabbit.oak.spi.security.user.AuthorizableType; +import org.apache.jackrabbit.oak.spi.security.user.util.UserUtil; +import org.jetbrains.annotations.NotNull; + +import static org.apache.jackrabbit.guava.common.base.Preconditions.checkNotNull; + +/** + * <code>GroupMembershipReader</code>... + */ +class GroupMembershipReader { + + private final MembershipProvider membershipProvider; + + protected final GroupPrincipalFactory groupPrincipalFactory; Review Comment: why is this protected? i don't see any need for a sub-class outside of this very package that would need to access this field. in fact i would prefer to have the subclass access it through a package-accessible method. ########## oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserPrincipalProvider.java: ########## @@ -209,6 +203,21 @@ public Iterator<? extends Principal> findPrincipals(int searchType) { } //------------------------------------------------------------< private >--- + + GroupPrincipalFactory createGroupPrincipalFactory() { Review Comment: return value should be annotated with @NotNull ########## oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/CachedGroupMembershipReader.java: ########## @@ -0,0 +1,209 @@ +/* + * 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 java.security.Principal; +import java.util.LinkedHashMap; +import java.util.Map; +import java.util.Objects; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.function.BiConsumer; + +import javax.jcr.AccessDeniedException; + +import org.apache.jackrabbit.guava.common.base.Joiner; +import org.apache.jackrabbit.guava.common.base.Strings; +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.oak.spi.security.user.AuthorizableType; +import org.apache.jackrabbit.oak.spi.security.user.UserConfiguration; +import org.apache.jackrabbit.oak.spi.security.user.util.UserUtil; +import org.apache.jackrabbit.util.Text; +import org.jetbrains.annotations.NotNull; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import static org.apache.jackrabbit.guava.common.base.Preconditions.checkNotNull; +import static org.apache.jackrabbit.oak.security.user.CacheConstants.REP_GROUP_PRINCIPAL_NAMES; +import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.EXPIRATION_NO_CACHE; +import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.NO_STALE_CACHE; +import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.PARAM_CACHE_EXPIRATION; +import static org.apache.jackrabbit.oak.security.user.UserPrincipalProvider.PARAM_CACHE_MAX_STALE; + +/** + * <code>CachedGroupMembershipReader</code>... + */ +class CachedGroupMembershipReader extends GroupMembershipReader { + + private static final Logger LOG = LoggerFactory.getLogger(CachedGroupMembershipReader.class); + + private static final long MEMBERSHIP_THRESHOLD = 0; + + /** + * Keep track of cache updates for 100 most recent authorizables. + */ + private static final int MAX_CACHE_TRACKING_ENTRIES = 100; + + private static final Map<UserConfiguration, Map<String, Long>> CACHE_UPDATES = new ConcurrentHashMap<>(); + + private final UserConfiguration config; + + private final Root root; + + private final long expiration; + private final long maxStale; + + CachedGroupMembershipReader(@NotNull MembershipProvider membershipProvider, + @NotNull GroupPrincipalFactory groupPrincipalFactory, + @NotNull UserConfiguration config, + @NotNull Root root) { + super(membershipProvider, groupPrincipalFactory); + this.config = checkNotNull(config); + this.root = root; + this.expiration = config.getParameters().getConfigValue(PARAM_CACHE_EXPIRATION, EXPIRATION_NO_CACHE); + this.maxStale = config.getParameters().getConfigValue(PARAM_CACHE_MAX_STALE, NO_STALE_CACHE); + } + + @Override + void getMembership(@NotNull Tree authorizable, + @NotNull Set<Principal> groupPrincipals) { + // membership cache only implemented on user + if (UserUtil.isType(authorizable, AuthorizableType.USER)) { + readGroupsFromCache(authorizable, groupPrincipals, this::loadGroupPrincipals); + } else { + loadGroupPrincipals(authorizable, groupPrincipals); + } + } + + private void readGroupsFromCache(@NotNull Tree authorizableNode, + @NotNull Set<Principal> groups, + @NotNull BiConsumer<Tree, Set<Principal>> loader) { + long expirationTime = 0; + String authorizablePath = authorizableNode.getPath(); + Tree principalCache = authorizableNode.getChild(CacheConstants.REP_CACHE); + if (principalCache.exists()) { + expirationTime = TreeUtil.getLong(principalCache, CacheConstants.REP_EXPIRATION, EXPIRATION_NO_CACHE); + } + long now = System.currentTimeMillis(); + if (expirationTime > EXPIRATION_NO_CACHE && now < expirationTime) { + serveGroupsFromCache(authorizablePath, principalCache, groups); + return; + } + + // need to load or serve stale if allowed + boolean mayServeStale = now - expirationTime < maxStale; + boolean updateCache; + Map<String, Long> updates = getCacheUpdateMap(); + synchronized (updates) { + // anyone else updating that entry already? + Long exp = updates.get(authorizablePath); + updateCache = exp == null || expirationTime > exp; + if (updateCache) { + // we do it + updates.put(authorizablePath, expirationTime); + } + } + + if (!updateCache && mayServeStale) { + LOG.debug("Another thread is updating the cache and we may serve stale"); + serveGroupsFromCache(authorizablePath, principalCache, groups); + return; + } + + if (updateCache) { + boolean cacheSuccess = false; + try { + loader.accept(authorizableNode, groups); + cacheGroups(authorizableNode, groups); + cacheSuccess = true; + } catch (AccessDeniedException | CommitFailedException e) { + LOG.debug("Failed to cache group membership: {}", e.getMessage()); + } finally { + if (!cacheSuccess || expirationTime == 0) { + synchronized (updates) { + Long exp = updates.get(authorizablePath); + if (Objects.equals(exp, expirationTime)) { + updates.remove(authorizablePath); + } + } + } + } + } else { + // load but do not cache. happens when another thread is updating + // the cache and this thread is not allowed to serve stale + LOG.debug("Load but do not cache. Another thread is updating the cache and this thread is not allowed to serve stale"); Review Comment: same applies here. if i were an outsider i would not understand what is 'stale' and what the implications are. ########## oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserPrincipalProvider.java: ########## @@ -209,6 +203,21 @@ public Iterator<? extends Principal> findPrincipals(int searchType) { } //------------------------------------------------------------< private >--- + + GroupPrincipalFactory createGroupPrincipalFactory() { Review Comment: missing annotation of return value (adding notnull would be the correct one afaik). i am wondering why this method is not private. IMHO nobody outside this class should use it. -- 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: dev-unsubscr...@jackrabbit.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
