From df7349b6e3855a2bb7ae46e89c57455d2b87f7af Mon Sep 17 00:00:00 2001 From: Derek Hulley Date: Sun, 8 Sep 2013 22:21:11 +0000 Subject: [PATCH] Merged DEV to HEAD: 52797: Fixed ALF-19301 (CLOUD-1685): Unsafe usage of transactions around authenticationCache 54967: Fix test after changes for ALF-19301: Unsafe usage of transactions around authenticationCache Merge: Note that the test changes were applied by using 'patch' because of the file relocation git-svn-id: https://svn.alfresco.com/repos/alfresco-enterprise/alfresco/HEAD/root@55090 c4b6b30b-aa2e-2d43-bbcb-ca4b014f7261 --- .../RepositoryAuthenticationDao.java | 79 +++++++------ .../authentication/AuthenticationTest.java | 104 ++++++++++++++---- 2 files changed, 128 insertions(+), 55 deletions(-) diff --git a/source/java/org/alfresco/repo/security/authentication/RepositoryAuthenticationDao.java b/source/java/org/alfresco/repo/security/authentication/RepositoryAuthenticationDao.java index 7f6cf71c22..fb2d02175e 100644 --- a/source/java/org/alfresco/repo/security/authentication/RepositoryAuthenticationDao.java +++ b/source/java/org/alfresco/repo/security/authentication/RepositoryAuthenticationDao.java @@ -40,7 +40,6 @@ import org.alfresco.repo.node.NodeServicePolicies.OnUpdatePropertiesPolicy; import org.alfresco.repo.policy.JavaBehaviour; import org.alfresco.repo.policy.PolicyComponent; import org.alfresco.repo.tenant.TenantService; -import org.alfresco.repo.transaction.AlfrescoTransactionSupport; import org.alfresco.repo.transaction.RetryingTransactionHelper.RetryingTransactionCallback; import org.alfresco.service.cmr.repository.ChildAssociationRef; import org.alfresco.service.cmr.repository.NodeRef; @@ -67,8 +66,8 @@ import org.springframework.dao.DataAccessException; public class RepositoryAuthenticationDao implements MutableAuthenticationDao, InitializingBean, OnUpdatePropertiesPolicy, BeforeDeleteNodePolicy { private static final StoreRef STOREREF_USERS = new StoreRef("user", "alfrescoUserStore"); - - private static final Log logger = LogFactory.getLog(RepositoryAuthenticationDao.class); + + private static Log logger = LogFactory.getLog(RepositoryAuthenticationDao.class); protected AuthorityService authorityService; protected NodeService nodeService; @@ -187,17 +186,50 @@ public class RepositoryAuthenticationDao implements MutableAuthenticationDao, In return userEntry == null ? null: userEntry.nodeRef; } + /** + * Get the cache entry (or cache it) if the user exists + * + * @param caseSensitiveSearchUserName the username to search for + * @return the user's data + */ private CacheEntry getUserEntryOrNull(final String caseSensitiveSearchUserName) { + if (caseSensitiveSearchUserName == null || caseSensitiveSearchUserName.length() == 0) + { + return null; + } + class SearchUserNameCallback implements RetryingTransactionCallback { @Override public CacheEntry execute() throws Throwable { - List results = nodeService.getChildAssocs(getUserFolderLocation(caseSensitiveSearchUserName), - ContentModel.ASSOC_CHILDREN, QName.createQName(ContentModel.USER_MODEL_URI, caseSensitiveSearchUserName)); + CacheEntry cacheEntry = authenticationCache.get(caseSensitiveSearchUserName); + + // Check the cache entry if it exists + if (cacheEntry != null && !nodeService.exists(cacheEntry.nodeRef)) + { + logger.warn("Detected state cache entry for '" + caseSensitiveSearchUserName + "'. Node does not exist: " + cacheEntry); + // We were about to give out a stale node. Something went wrong with the cache. + // The removal is guaranteed whether we commit or rollback. + removeAuthenticationFromCache(caseSensitiveSearchUserName); + cacheEntry = null; + } + // Check again + if (cacheEntry != null) + { + // We found what we wanted + return cacheEntry; + } + + // Not found, so query + List results = nodeService.getChildAssocs( + getUserFolderLocation(caseSensitiveSearchUserName), + ContentModel.ASSOC_CHILDREN, + QName.createQName(ContentModel.USER_MODEL_URI, caseSensitiveSearchUserName)); if (!results.isEmpty()) { + // Extract values from the query results NodeRef userRef = tenantService.getName(results.get(0).getChildRef()); Map properties = nodeService.getProperties(userRef); String password = DefaultTypeConverter.INSTANCE.convert(String.class, @@ -224,41 +256,16 @@ public class RepositoryAuthenticationDao implements MutableAuthenticationDao, In !getLocked(userName, properties, isAdminAuthority), gas); - return new CacheEntry(userRef, ud, credentialsExpiryDate); + cacheEntry = new CacheEntry(userRef, ud, credentialsExpiryDate); + // Only cache positive results + authenticationCache.put(caseSensitiveSearchUserName, cacheEntry); } - return null; + return cacheEntry; } } - if (caseSensitiveSearchUserName == null || caseSensitiveSearchUserName.length() == 0) - { - return null; - } - - CacheEntry result = authenticationCache.get(caseSensitiveSearchUserName); - if (result == null) - { - if (AlfrescoTransactionSupport.getTransactionId() == null) - { - result = transactionService.getRetryingTransactionHelper().doInTransaction(new SearchUserNameCallback()); - } - else - { - try - { - result = new SearchUserNameCallback().execute(); - } - catch (Throwable e) - { - logger.error(e); - } - } - if (result != null) - { - authenticationCache.put(caseSensitiveSearchUserName, result); - } - } - return result; + // Always use a transaction + return transactionService.getRetryingTransactionHelper().doInTransaction(new SearchUserNameCallback(), true); } @Override diff --git a/source/test-java/org/alfresco/repo/security/authentication/AuthenticationTest.java b/source/test-java/org/alfresco/repo/security/authentication/AuthenticationTest.java index 7cbd328176..99184f745d 100644 --- a/source/test-java/org/alfresco/repo/security/authentication/AuthenticationTest.java +++ b/source/test-java/org/alfresco/repo/security/authentication/AuthenticationTest.java @@ -45,6 +45,7 @@ import org.alfresco.error.AlfrescoRuntimeException; import org.alfresco.model.ContentModel; import org.alfresco.repo.cache.SimpleCache; import org.alfresco.repo.management.subsystems.ChildApplicationContextManager; +import org.alfresco.repo.policy.BehaviourFilter; import org.alfresco.repo.policy.PolicyComponent; import org.alfresco.repo.security.authentication.AuthenticationUtil.RunAsWork; import org.alfresco.repo.security.authentication.InMemoryTicketComponentImpl.ExpiryMode; @@ -103,6 +104,7 @@ public class AuthenticationTest extends TestCase private Dialect dialect; private PolicyComponent policyComponent; + private BehaviourFilter behaviourFilter; private SimpleCache authenticationCache; private SimpleCache immutableSingletonCache; @@ -141,6 +143,7 @@ public class AuthenticationTest extends TestCase pubPersonService = (PersonService) ctx.getBean("PersonService"); personService = (PersonService) ctx.getBean("personService"); policyComponent = (PolicyComponent) ctx.getBean("policyComponent"); + behaviourFilter = (BehaviourFilter) ctx.getBean("policyBehaviourFilter"); authenticationCache = (SimpleCache) ctx.getBean("authenticationCache"); immutableSingletonCache = (SimpleCache) ctx.getBean("immutableSingletonCache"); // permissionServiceSPI = (PermissionServiceSPI) @@ -155,6 +158,26 @@ public class AuthenticationTest extends TestCase authenticationManager = (AuthenticationManager) subsystem.getBean("authenticationManager"); transactionService = (TransactionService) ctx.getBean(ServiceRegistry.TRANSACTION_SERVICE.getLocalName()); + + // Clean up before we start trying to create the test user + transactionService.getRetryingTransactionHelper().doInTransaction(new RetryingTransactionCallback() + { + @Override + public Void execute() throws Throwable + { + AuthenticationUtil.setFullyAuthenticatedUser(AuthenticationUtil.getSystemUserName()); + try + { + deleteAndy(); + return null; + } + finally + { + authenticationComponent.clearCurrentSecurityContext(); + } + } + }, false, true); + userTransaction = transactionService.getUserTransaction(); userTransaction.begin(); @@ -174,13 +197,13 @@ public class AuthenticationTest extends TestCase AuthenticationUtil.setFullyAuthenticatedUser(AuthenticationUtil.getSystemUserName()); - deleteAndy(); authenticationComponent.clearCurrentSecurityContext(); } private void deleteAndy() { RepositoryAuthenticationDao dao = new RepositoryAuthenticationDao(); + dao.setTransactionService(transactionService); dao.setAuthorityService(authorityService); dao.setTenantService(tenantService); dao.setNodeService(nodeService); @@ -191,16 +214,23 @@ public class AuthenticationTest extends TestCase dao.setAuthenticationCache(authenticationCache); dao.setSingletonCache(immutableSingletonCache); - if (dao.getUserOrNull("andy") != null) + if (dao.userExists("andy")) { dao.deleteUser("andy"); } + if (dao.userExists("Andy")) + { + dao.deleteUser("Andy"); + } - if(personService.personExists("andy")) + if (personService.personExists("andy")) { personService.deletePerson("andy"); } - + if (personService.personExists("Andy")) + { + personService.deletePerson("Andy"); + } } @Override @@ -316,18 +346,6 @@ public class AuthenticationTest extends TestCase authenticationComponent.clearCurrentSecurityContext(); } - public void c() - { - try - { - authenticationService.authenticate("", "".toCharArray()); - } - catch (AuthenticationException e) - { - // Expected - } - } - public void testNewTicketOnLogin() { authenticationComponent.setSystemUserAsCurrentUser(); @@ -402,10 +420,11 @@ public class AuthenticationTest extends TestCase // tenant domain ~,./<>?\\| is not valid format" } } - - public void testCreateAndyUserAndOtherCRUD() throws NoSuchAlgorithmException, UnsupportedEncodingException + + private RepositoryAuthenticationDao createRepositoryAuthenticationDao() { RepositoryAuthenticationDao dao = new RepositoryAuthenticationDao(); + dao.setTransactionService(transactionService); dao.setTenantService(tenantService); dao.setNodeService(nodeService); dao.setAuthorityService(authorityService); @@ -415,6 +434,12 @@ public class AuthenticationTest extends TestCase dao.setPolicyComponent(policyComponent); dao.setAuthenticationCache(authenticationCache); dao.setSingletonCache(immutableSingletonCache); + return dao; + } + + public void testCreateAndyUserAndOtherCRUD() throws NoSuchAlgorithmException, UnsupportedEncodingException + { + RepositoryAuthenticationDao dao = createRepositoryAuthenticationDao(); dao.createUser("Andy", "cabbage".toCharArray()); assertNotNull(dao.getUserOrNull("Andy")); @@ -452,7 +477,8 @@ public class AuthenticationTest extends TestCase // assertNotSame(oldSalt, dao.getSalt(newDetails)); dao.deleteUser("Andy"); - assertNull(dao.getUserOrNull("Andy")); + assertFalse("Should not be a cache entry for 'Andy'.", authenticationCache.contains("Andy")); + assertNull("DAO should report that 'Andy' does not exist.", dao.getUserOrNull("Andy")); MessageDigest digester; try @@ -466,7 +492,47 @@ public class AuthenticationTest extends TestCase e.printStackTrace(); System.out.println("No digester"); } + } + + /** + * ALF-19301: Unsafe usage of transactions around authenticationCache + */ + public void testStaleAuthenticationCacheRecovery() + { + RepositoryAuthenticationDao dao = createRepositoryAuthenticationDao(); + + assertFalse("Must start with no cache entry for 'Andy'.", authenticationCache.contains("Andy")); + dao.createUser("Andy", "cabbage".toCharArray()); + NodeRef andyNodeRef = dao.getUserOrNull("Andy"); + assertNotNull(andyNodeRef); + assertTrue("Andy's node should exist. ", nodeService.exists(andyNodeRef)); + + // So the cache should be populated. Now, remove Andy's node but without having policies fire. + behaviourFilter.disableBehaviour(andyNodeRef); + nodeService.deleteNode(andyNodeRef); + + assertTrue("Should still have an entry for 'Andy'.", authenticationCache.contains("Andy")); + + assertNull("Invalid node should be detected for 'Andy'.", dao.getUserOrNull("Andy")); + assertFalse("Cache entry should have been removed for 'Andy'.", authenticationCache.contains("Andy")); + } + + /** + * Test for use without txn. + */ + public void testRepositoryAuthenticationDaoWithoutTxn() throws Exception + { + RepositoryAuthenticationDao dao = createRepositoryAuthenticationDao(); + + dao.createUser("Andy", "cabbage".toCharArray()); + authenticationCache.remove("Andy"); // Make sure we query + + this.userTransaction.commit(); + + // Now get the user out of a transaction + dao.userExists("Andy"); + assertTrue("Should now have an entry for 'Andy'.", authenticationCache.contains("Andy")); } public void testAuthentication()