From e1e306d2197dabfa93b0bc935ed391888eaabdf2 Mon Sep 17 00:00:00 2001 From: Alan Davis Date: Sat, 31 Jan 2015 10:09:03 +0000 Subject: [PATCH] Merged HEAD-BUG-FIX (5.1/Cloud) to HEAD (5.0/Cloud) 88886: Merged V4.2-BUG-FIX (4.2.4) to HEAD-BUG-FIX (5.0/Cloud) 88846: MNT-12473: Merged V4.2.2 (4.2.2.16) to V4.2-BUG-FIX (4.2.4) 88606: MNT-12574: CLONE - cyclic groups can be created bringing the server down During build of the AuthorityBridgeTableAsynchronouslyRefreshedCache check and fix all cyclic groups. Add unit test. 88847: MNT-12473: Merged V4.2.2 (4.2.2.16) to V4.2-BUG-FIX (4.2.4) 88650: MNT-12574: CLONE - cyclic groups can be created bringing the server down Do the cyclic check only in case of ConcurrentModificationException. git-svn-id: https://svn.alfresco.com/repos/alfresco-enterprise/alfresco/HEAD/root@94588 c4b6b30b-aa2e-2d43-bbcb-ca4b014f7261 --- config/alfresco/cache-context.xml | 1 + ...idgeTableAsynchronouslyRefreshedCache.java | 142 +++++++++++++++++- .../repo/security/authority/AuthorityDAO.java | 5 + .../security/authority/AuthorityDAOImpl.java | 10 +- ...TableAsynchronouslyRefreshedCacheTest.java | 104 ++++++++++++- 5 files changed, 257 insertions(+), 5 deletions(-) diff --git a/config/alfresco/cache-context.xml b/config/alfresco/cache-context.xml index 2b4a8d34cf..64ba00b281 100644 --- a/config/alfresco/cache-context.xml +++ b/config/alfresco/cache-context.xml @@ -43,6 +43,7 @@ + diff --git a/source/java/org/alfresco/repo/security/authority/AuthorityBridgeTableAsynchronouslyRefreshedCache.java b/source/java/org/alfresco/repo/security/authority/AuthorityBridgeTableAsynchronouslyRefreshedCache.java index 86025b8c61..6a1e324222 100644 --- a/source/java/org/alfresco/repo/security/authority/AuthorityBridgeTableAsynchronouslyRefreshedCache.java +++ b/source/java/org/alfresco/repo/security/authority/AuthorityBridgeTableAsynchronouslyRefreshedCache.java @@ -1,5 +1,5 @@ /* - * Copyright (C) 2005-2013 Alfresco Software Limited. + * Copyright (C) 2005-2014 Alfresco Software Limited. * * This file is part of Alfresco * @@ -18,8 +18,14 @@ */ package org.alfresco.repo.security.authority; +import java.util.ConcurrentModificationException; +import java.util.HashMap; +import java.util.HashSet; import java.util.List; +import java.util.Map; +import java.util.Set; +import org.alfresco.error.AlfrescoRuntimeException; import org.alfresco.repo.cache.AbstractMTAsynchronouslyRefreshedCache; import org.alfresco.repo.security.authentication.AuthenticationUtil; import org.alfresco.repo.security.authentication.AuthenticationUtil.RunAsWork; @@ -28,6 +34,8 @@ import org.alfresco.repo.transaction.RetryingTransactionHelper; import org.alfresco.repo.transaction.RetryingTransactionHelper.RetryingTransactionCallback; import org.alfresco.util.BridgeTable; import org.alfresco.util.PropertyCheck; +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; import org.springframework.beans.factory.InitializingBean; /** @@ -39,6 +47,18 @@ public class AuthorityBridgeTableAsynchronouslyRefreshedCache extends AbstractM private AuthorityBridgeDAO authorityBridgeDAO; private RetryingTransactionHelper retryingTransactionHelper; private TenantAdminService tenantAdminService; + private AuthorityDAO authorityDAO; + + private Log logger = LogFactory.getLog(getClass()); + + /** + * @param authorityDAO + * the authorityDAO to set + */ + public void setAuthorityDAO(AuthorityDAO authorityDAO) + { + this.authorityDAO = authorityDAO; + } /** * @param authorityBridgeDAO @@ -87,18 +107,134 @@ public class AuthorityBridgeTableAsynchronouslyRefreshedCache extends AbstractM { List links = authorityBridgeDAO.getAuthorityBridgeLinks(); BridgeTable bridgeTable = new BridgeTable(); - for (AuthorityBridgeLink link : links) + try { - bridgeTable.addLink(link.getParentName(), link.getChildName()); + for (AuthorityBridgeLink link : links) + { + bridgeTable.addLink(link.getParentName(), link.getChildName()); + } + } + catch (ConcurrentModificationException e) + { + // Explain exception + checkCyclic(links); + // If cyclic groups is not the cause then rethrow + throw e; } return bridgeTable; } + private void checkCyclic(List links) + { + Map> parentsToChildren = new HashMap>(); + for (AuthorityBridgeLink link : links) + { + addToMap(parentsToChildren, link.getParentName(), link.getChildName()); + } + + Map> removed = new HashMap>(); + for (String parent : parentsToChildren.keySet()) + { + if (logger.isDebugEnabled()) + { + logger.debug("Start checking from '" + parent + "'"); + } + Set authorities = new HashSet(); + authorities.add(parent); + doCheck(parent, parentsToChildren, authorities, removed); + } + if (!removed.isEmpty()) + { + fixCyclic(removed); + throw new AlfrescoRuntimeException("Cyclic links were detected and removed."); + } + } + + private void doCheck(String parent, Map> parentsToChildren, Set authorities, + Map> removed) + { + Set children = parentsToChildren.get(parent); + if (children != null) + { + for (String child : children) + { + if (logger.isDebugEnabled()) + { + logger.debug("Check link from '" + parent + "' to '" + child + "'"); + } + if (isRemoved(removed, parent, child)) + { + if (logger.isDebugEnabled()) + { + logger.debug("Link from '" + parent + "' to '" + child + "' has been already removed"); + } + continue; + } + if (!authorities.add(child)) + { + addToMap(removed, parent, child); + continue; + } + doCheck(child, parentsToChildren, authorities, removed); + authorities.remove(child); + } + if (logger.isDebugEnabled()) + { + logger.debug("Children of '" + parent + "' were processed"); + } + } + } + + private boolean isRemoved(Map> removed, String parent, String child) + { + Set remChildren = removed.get(parent); + return (remChildren != null && remChildren.contains(child)); + } + + private void addToMap(Map> map, String parent, String child) + { + Set children = map.get(parent); + if (children == null) + { + children = new HashSet(); + children.add(child); + map.put(parent, children); + } + else + { + children.add(child); + } + } + + private void fixCyclic(final Map> removed) + { + // delete cyclic links in new transaction because + // current cache refresh will be interrupted with AlfrescoRuntimeException + retryingTransactionHelper.doInTransaction(new RetryingTransactionCallback() + { + @Override + public Void execute() throws Throwable + { + for (String parentName : removed.keySet()) + { + for (String childName : removed.get(parentName)) + { + // do not refresh authorityBridgeTableCache + authorityDAO.removeAuthority(parentName, childName, false); + logger.error("Link from '" + parentName + "' to '" + childName +"' was removed to break cycle."); + } + } + return null; + } + }, false, true); + } + @Override public void afterPropertiesSet() throws Exception { PropertyCheck.mandatory(this, "authorityBridgeDAO", authorityBridgeDAO); PropertyCheck.mandatory(this, "retryingTransactionHelper", retryingTransactionHelper); + PropertyCheck.mandatory(this, "authorityDAO", authorityDAO); super.afterPropertiesSet(); } } diff --git a/source/java/org/alfresco/repo/security/authority/AuthorityDAO.java b/source/java/org/alfresco/repo/security/authority/AuthorityDAO.java index 581502d501..7ba055bc1e 100644 --- a/source/java/org/alfresco/repo/security/authority/AuthorityDAO.java +++ b/source/java/org/alfresco/repo/security/authority/AuthorityDAO.java @@ -73,6 +73,11 @@ public interface AuthorityDAO */ void removeAuthority(String parentName, String childName); + /** + * Remove an authority without authorityBridgeTableCache refresh. + */ + void removeAuthority(String parentName, String childName, boolean cacheRefresh); + /** * Get the authorities that contain the one given. */ diff --git a/source/java/org/alfresco/repo/security/authority/AuthorityDAOImpl.java b/source/java/org/alfresco/repo/security/authority/AuthorityDAOImpl.java index 3b228fc2c6..5e908cb4d9 100644 --- a/source/java/org/alfresco/repo/security/authority/AuthorityDAOImpl.java +++ b/source/java/org/alfresco/repo/security/authority/AuthorityDAOImpl.java @@ -772,6 +772,11 @@ public class AuthorityDAOImpl implements AuthorityDAO, NodeServicePolicies.Befor } public void removeAuthority(String parentName, String childName) + { + removeAuthority(parentName, childName, true); + } + + public void removeAuthority(String parentName, String childName, boolean cacheRefresh) { NodeRef parentRef = getAuthorityOrNull(parentName); if (parentRef == null) @@ -792,7 +797,10 @@ public class AuthorityDAOImpl implements AuthorityDAO, NodeServicePolicies.Befor else { userAuthorityCache.clear(); - authorityBridgeTableCache.refresh(); + if (cacheRefresh) + { + authorityBridgeTableCache.refresh(); + } } } diff --git a/source/test-java/org/alfresco/repo/security/authority/AuthorityBridgeTableAsynchronouslyRefreshedCacheTest.java b/source/test-java/org/alfresco/repo/security/authority/AuthorityBridgeTableAsynchronouslyRefreshedCacheTest.java index 7288be4d50..3978301e82 100644 --- a/source/test-java/org/alfresco/repo/security/authority/AuthorityBridgeTableAsynchronouslyRefreshedCacheTest.java +++ b/source/test-java/org/alfresco/repo/security/authority/AuthorityBridgeTableAsynchronouslyRefreshedCacheTest.java @@ -1,5 +1,5 @@ /* - * Copyright (C) 2005-2013 Alfresco Software Limited. + * Copyright (C) 2005-2014 Alfresco Software Limited. * * This file is part of Alfresco * @@ -18,6 +18,15 @@ */ package org.alfresco.repo.security.authority; +import static org.mockito.Matchers.anyString; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.anyBoolean; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.util.ConcurrentModificationException; +import java.util.LinkedList; +import java.util.List; import java.util.Set; import junit.framework.TestCase; @@ -37,7 +46,10 @@ import org.alfresco.service.transaction.TransactionService; import org.alfresco.test_category.OwnJVMTestsCategory; import org.alfresco.util.ApplicationContextHelper; import org.alfresco.util.GUID; + import org.junit.experimental.categories.Category; +import org.mockito.invocation.InvocationOnMock; +import org.mockito.stubbing.Answer; import org.springframework.context.ApplicationContext; @Category(OwnJVMTestsCategory.class) @@ -154,4 +166,94 @@ public class AuthorityBridgeTableAsynchronouslyRefreshedCacheTest extends TestCa } }, false, true); } + + /** + * See MNT-12473 + */ + public void testCyclicGroups() + { + List cyclicLinks = new LinkedList(); + // no cycle + cyclicLinks.add(createAuthorityBridgeLink("a1", "a2")); + + cyclicLinks.add(createAuthorityBridgeLink("g1", "g2")); + cyclicLinks.add(createAuthorityBridgeLink("g2", "g3")); + cyclicLinks.add(createAuthorityBridgeLink("g3", "g4")); + // 1st cycle + cyclicLinks.add(createAuthorityBridgeLink("g4", "g1")); + + cyclicLinks.add(createAuthorityBridgeLink("b1", "b2")); + // child with no cycle + cyclicLinks.add(createAuthorityBridgeLink("b2", "a1")); + cyclicLinks.add(createAuthorityBridgeLink("b2", "b3")); + // 2nd cycle + cyclicLinks.add(createAuthorityBridgeLink("b3", "b1")); + + cyclicLinks.add(createAuthorityBridgeLink("d1", "d2")); + cyclicLinks.add(createAuthorityBridgeLink("d2", "d3")); + // 3rd cycle + cyclicLinks.add(createAuthorityBridgeLink("d3", "d1")); + cyclicLinks.add(createAuthorityBridgeLink("d2", "d4")); + // 4th cycle + cyclicLinks.add(createAuthorityBridgeLink("d4", "d1")); + cyclicLinks.add(createAuthorityBridgeLink("d3", "d5")); + // 5th cycle + cyclicLinks.add(createAuthorityBridgeLink("d5", "d1")); + + AuthorityBridgeDAO authorityBridgeDAOMock = mock(AuthorityBridgeDAO.class); + when(authorityBridgeDAOMock.getAuthorityBridgeLinks()).thenReturn(cyclicLinks); + + AuthorityDAO authorityDAOMock = mock(AuthorityDAO.class); + class Counter + { + private int removed = 0; + public int getRemoved() + { + return removed; + } + public void increase() + { + this.removed++; + } + } + ; + final Counter cycles = new Counter(); + doAnswer(new Answer() + { + public Object answer(InvocationOnMock invocation) + { + cycles.increase(); + return null; + } + }).when(authorityDAOMock).removeAuthority(anyString(), anyString(), anyBoolean()); + + AuthorityBridgeTableAsynchronouslyRefreshedCache cache = new AuthorityBridgeTableAsynchronouslyRefreshedCache(); + cache.setAuthorityBridgeDAO(authorityBridgeDAOMock); + cache.setAuthorityDAO(authorityDAOMock); + cache.setTenantAdminService(tenantAdminService); + cache.setRetryingTransactionHelper(transactionService.getRetryingTransactionHelper()); + + try + { + cache.buildCache(tenantAdminService.getCurrentUserDomain()); + } + catch (AlfrescoRuntimeException e1) + { + assertTrue(e1.getMessage().contains("Cyclic links were detected")); + assertEquals(5, cycles.getRemoved()); + } + catch (ConcurrentModificationException e2) + { + fail("Cyclic links were NOT detected and processed"); + } + } + + private AuthorityBridgeLink createAuthorityBridgeLink(String parentName, String childName) + { + AuthorityBridgeLink link = new AuthorityBridgeLink(); + link.setParentName(parentName); + link.setChildName(childName); + return link; + } + }