OAK-11111 Check if principal cache property exists (#1715)

* OAK-11111 Check if principal cache property exists

Added a condition to check if the principal cache value it's already present

* OAK-11111  Extended test coverage

oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/CachedPrincipalMembershipReader.java

@@ -131,17 +131,18 @@ private void readGroupsFromCache(@NotNull Tree authorizableTree,
                 cacheGroups(authorizableTree, groups);
             } else {
                 // another thread is already updating the cache, which leaves two options
-                // 1. serve a stale cache if allowed
+                // 1. serve a stale cache if allowed and if the cache values exist
                 // 2. read membership from membership-provider without caching the result
-                if (canServeStaleCache(expirationTime, now)) {
+                if (canServeStaleCache(expirationTime, now) && hasCacheValues(principalCache)) {
                     // the cache cannot be updated by the current thread but a stale cache can be returned and reading
                     // membership again can be avoided
                     LOG.debug("Another thread is updating the cache, returning a stale cache for '{}'.", authorizablePath);
                     serveGroupsFromCache(principalCache, groups);
                 } else {
-                    // another thread is updating the cache and this thread is not allowed to serve a stale cache,
-                    // therefore read membership from membership-provider but do not cache the result.
-                    LOG.debug("Another thread is updating the cache and this thread is not allowed to serve a stale cache; reading from persistence without caching.");
+                    // another thread is updating the cache and this thread is not allowed to serve a stale cache
+                    // because there's no cache or it's not allowed to serve stale cache
+                    // therefore read membership from membership-provider.
+                    LOG.debug("This thread is not allowed to serve a stale cache; reading from provider without caching.");
                     loader.accept(authorizableTree, groups);
                 }
             }
@@ -171,6 +172,11 @@ private boolean canServeStaleCache(long expirationTime, long now) {
         return now - expirationTime < maxStale;
     }
 
+    private boolean hasCacheValues(@NotNull Tree principalCache) {
+        return principalCache.exists() &&
+                !Strings.isNullOrEmpty(TreeUtil.getString(principalCache, REP_GROUP_PRINCIPAL_NAMES));
+    }
+
     /**
      * Populate the given set with the group principals read from the cache.
      *

oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/CachedPrincipalMembershipReaderTest.java

@@ -175,7 +175,7 @@ public void testReadMembershipForNonUser() throws Exception {
      */
     @Test
     public void testReadMembershipWithCache() throws Exception {
-        Root systemRoot = getSystemRoot();
+        Root systemRoot = spy(getSystemRoot());
         CachedPrincipalMembershipReader cachedGroupMembershipReader = createPrincipalMembershipReader(systemRoot);
 
         Set<Principal> groupPrincipal = new HashSet<>();
@@ -191,6 +191,8 @@ public void testReadMembershipWithCache() throws Exception {
         //Assert that the cache was used
         assertEquals("Reading membership from cache for '" + userPath + "'", logCustomizer.getLogs().get(2));
         assertEquals(1, groupPrincipal.size());
+        //Assert that the cache was written just once
+        verify(systemRoot, times(1)).commit(CacheValidatorProvider.asCommitAttributes());
     }
 
     /**
@@ -201,7 +203,7 @@ public void testReadMembershipWithCache() throws Exception {
      */
     @Test
     public void testCacheGroupMembershipGetMemberStaleCache() throws Exception {
-        initMocks();
+        initMocks(false);
 
         // Test getMembership from multiple threads on expired cache and verify that:
         // - only one thread updated the cache
@@ -223,15 +225,15 @@ public void testCacheGroupMembershipGetMemberStaleCache() throws Exception {
         verify(mockedRoot, times(1)).commit(CacheValidatorProvider.asCommitAttributes());
         if (cacheMaxStale == UserPrincipalProvider.NO_STALE_CACHE) {
             assertEquals(NUM_THREADS, logCustomizer.getLogs().size());
-            logCustomizer.getLogs().subList(0, NUM_THREADS - 1).forEach(s -> assertEquals("Another thread is updating the cache and this thread is not allowed to serve a stale cache; reading from persistence without caching.", s));
+            logCustomizer.getLogs().subList(0, NUM_THREADS - 1).forEach(s -> assertEquals("This thread is not allowed to serve a stale cache; reading from provider without caching.", s));
         } else {
             assertEquals(NUM_THREADS, logCustomizer.getLogs().size());
             logCustomizer.getLogs().subList(0, NUM_THREADS - 1).forEach(s -> assertEquals("Another thread is updating the cache, returning a stale cache for '" + mockedUser.getPath() + "'.", s));
         }
         assertTrue(logCustomizer.getLogs().get(NUM_THREADS - 1).startsWith("Cached membership at " + mockedUser.getPath()));
     }
 
-    private void initMocks() throws CommitFailedException {
+    private void initMocks(boolean emptyCache) throws CommitFailedException {
         mockedRoot = mock(Root.class);
         doAnswer(invocation -> {
             Thread.sleep(3000);
@@ -258,7 +260,12 @@ private void initMocks() throws CommitFailedException {
 
         PropertyState propertyStatePrincipalNames = mock(PropertyState.class);
         when(propertyStatePrincipalNames.getValue(Type.STRING)).thenReturn("groupPrincipal");
-        when(mockedPrincipalCache.getProperty(CacheConstants.REP_GROUP_PRINCIPAL_NAMES)).thenReturn(propertyStatePrincipalNames);
+        if (!emptyCache) {
+            // Set the property to indicate that the cache is valid
+            when(mockedPrincipalCache.hasProperty(CacheConstants.REP_GROUP_PRINCIPAL_NAMES)).thenReturn(true);
+            when(mockedPrincipalCache.getProperty(CacheConstants.REP_GROUP_PRINCIPAL_NAMES)).thenReturn(
+                    propertyStatePrincipalNames);
+        }
 
         when(mockedUser.getChild(CacheConstants.REP_CACHE)).thenReturn(mockedPrincipalCache);
 
@@ -327,4 +334,34 @@ public void testMaxCacheTrackingEntries() throws Exception {
         }
     }
 
+    @Test
+    public void testCacheBeingBuiltReturnCallsOriginalProvider() throws Exception {
+        {
+            initMocks(true);
+
+            // Test getMembership from multiple threads on expired cache and verify that:
+            // - only one thread updated the cache
+            // - the stale value was provided
+            Thread[] getMembershipThreads = new Thread[NUM_THREADS];
+            for (int i = 0; i < getMembershipThreads.length; i++) {
+                getMembershipThreads[i] = new Thread(() -> {
+                    Set<Principal> groupPrincipals = new HashSet<>();
+                    CachedPrincipalMembershipReader cachedGroupMembershipReader = new CachedPrincipalMembershipReader(mockedMembershipProvider, mockedGroupPrincipalFactory, getUserConfiguration(), mockedRoot);
+                    cachedGroupMembershipReader.readMembership(mockedUser, groupPrincipals);
+                    assertEquals(groupPrincipals.size(),1);
+                });
+                getMembershipThreads[i].start();
+            }
+            for (Thread getMembershipThread : getMembershipThreads) {
+                getMembershipThread.join();
+            }
+
+            verify(mockedRoot, times(1)).commit(CacheValidatorProvider.asCommitAttributes());
+            assertEquals(NUM_THREADS, logCustomizer.getLogs().size());
+            logCustomizer.getLogs().subList(0, NUM_THREADS - 1).forEach(s -> assertEquals("This thread is not allowed to serve a stale cache; reading from provider without caching.", s));
+            assertTrue(logCustomizer.getLogs().get(NUM_THREADS - 1).startsWith("Cached membership at " + mockedUser.getPath()));
+        }
+    }
+
+
 }