From: Patrick Schmitz Date: Fri, 1 Feb 2013 21:47:09 +0000 (-0800) Subject: CSPACE-5741 - Fixed bugs in import that were not correcting detecting existing entrie... X-Git-Url: https://git.aero2k.de/?a=commitdiff_plain;h=9a3a252f81cb3c6e89824d80a8e630514ff574c6;p=tmp%2Fjakarta-migration.git CSPACE-5741 - Fixed bugs in import that were not correcting detecting existing entries in the roles and permissionRoleRel tables. Improved logging around the exists test, for more meaningful trace output during import. Fix another bug in the SpringPermissionManager.java that was not checking for existing ace's before adding new ones. This led to duplicate entries (not an error, but inefficient) when ant import was run multiple times. --- diff --git a/services/authentication/service/src/main/java/org/collectionspace/authentication/spring/SpringAuthNContext.java b/services/authentication/service/src/main/java/org/collectionspace/authentication/spring/SpringAuthNContext.java index 27801a2b4..17b59baa6 100644 --- a/services/authentication/service/src/main/java/org/collectionspace/authentication/spring/SpringAuthNContext.java +++ b/services/authentication/service/src/main/java/org/collectionspace/authentication/spring/SpringAuthNContext.java @@ -78,6 +78,7 @@ final public class SpringAuthNContext extends AuthNContext { if (caller == null) { String msg = "Could not find Subject!"; //TODO: find out why subject is not null + // Can happen during ant import, when there is no session/user. //FIXME: if logger is loaded when authn comes up, use it //logger.warn(msg); System.err.println(msg); diff --git a/services/authorization-mgt/import/src/main/resources/log4j.properties b/services/authorization-mgt/import/src/main/resources/log4j.properties index b24525085..79746b946 100644 --- a/services/authorization-mgt/import/src/main/resources/log4j.properties +++ b/services/authorization-mgt/import/src/main/resources/log4j.properties @@ -19,6 +19,7 @@ log4j.appender.R.layout.ConversionPattern=%d %-5p [%t] [%c:%L] %m%n #packages log4j.logger.org.collectionspace=DEBUG log4j.logger.org.collectionspace.services.authorization.spring=INFO +# log4j.logger.org.collectionspace.services.common.authorization_mgt.AuthorizationStore=TRACE log4j.logger.org.apache=INFO log4j.logger.httpclient=INFO log4j.logger.org.jboss.resteasy=INFO @@ -29,3 +30,4 @@ log4j.logger.org.springframework=INFO log4j.logger.ch.elca.el4j.services.xmlmerge=INFO log4j.logger.com.sun.xml.bind.v2.runtime=DEBUG log4j.logger.javax.persistence.PersistenceException=TRACE + diff --git a/services/authorization/service/src/main/java/org/collectionspace/services/authorization/spring/SpringPermissionManager.java b/services/authorization/service/src/main/java/org/collectionspace/services/authorization/spring/SpringPermissionManager.java index 8518a92db..81324bc13 100644 --- a/services/authorization/service/src/main/java/org/collectionspace/services/authorization/spring/SpringPermissionManager.java +++ b/services/authorization/service/src/main/java/org/collectionspace/services/authorization/spring/SpringPermissionManager.java @@ -320,16 +320,41 @@ public class SpringPermissionManager implements CSpacePermissionManager { } acl = provider.getProviderAclService().createAcl(oid); } - acl.insertAce(acl.getEntries().size(), permission, sid, grant); - provider.getProviderAclService().updateAcl(acl); + // Need to see if there is already an entry, so we do not duplicate (e.g., + // when we run our permission-roles init more than once. + List aceEntries = acl.getEntries(); + if(aceListHasEntry(aceEntries, permission, sid, grant)) { + if (log.isDebugEnabled()) { + log.debug("addPermission: Pre-existing acl for oid=" + oid.toString() + + " perm=" + permission.toString() + + " sid=" + sid.toString() + + " grant=" + grant); + } + + } else { + acl.insertAce(acl.getEntries().size(), permission, sid, grant); + provider.getProviderAclService().updateAcl(acl); - if (log.isDebugEnabled()) { - log.debug("addPermission: added acl for oid=" + oid.toString() - + " perm=" + permission.toString() - + " sid=" + sid.toString() - + " grant=" + grant); + if (log.isDebugEnabled()) { + log.debug("addPermission: added acl for oid=" + oid.toString() + + " perm=" + permission.toString() + + " sid=" + sid.toString() + + " grant=" + grant); + } } } + + private boolean aceListHasEntry(List aceEntries, Permission permission, + Sid sid, boolean grant) { + for(AccessControlEntry entry : aceEntries) { + if(permission.equals(entry.getPermission()) + && sid.equals(entry.getSid()) + && grant == entry.isGranting()) { + return true; + } + } + return false; + } /** * deletePermissions deletes given permission on given object id for given sid diff --git a/services/common/src/main/java/org/collectionspace/services/common/authorization_mgt/AuthorizationStore.java b/services/common/src/main/java/org/collectionspace/services/common/authorization_mgt/AuthorizationStore.java index 3ff03ee6d..7aff76a86 100644 --- a/services/common/src/main/java/org/collectionspace/services/common/authorization_mgt/AuthorizationStore.java +++ b/services/common/src/main/java/org/collectionspace/services/common/authorization_mgt/AuthorizationStore.java @@ -32,6 +32,7 @@ import javax.persistence.EntityManager; import javax.persistence.EntityManagerFactory; import org.collectionspace.services.authorization.Role; +import org.collectionspace.services.authorization.PermissionRoleRel; import org.collectionspace.services.authorization.perms.Permission; import org.collectionspace.services.common.authorization_mgt.RoleStorageConstants; import org.collectionspace.services.common.document.JaxbUtils; @@ -79,6 +80,25 @@ public class AuthorizationStore { } + static public PermissionRoleRel getPermRoleRel(EntityManager em, String permId, String roleId) { + PermissionRoleRel permRoleRel = null; + + try { + permRoleRel = (PermissionRoleRel)JpaStorageUtils.getEntityByDualKeys(em, + PermissionRoleRel.class.getName(), + RoleStorageConstants.PERM_ROLE_REL_PERM_ID, permId, + RoleStorageConstants.PERM_ROLE_REL_ROLE_ID, roleId); + } catch (Throwable e) { + if (logger.isTraceEnabled()) { + logger.trace("Could not retrieve permissionRoleRel with permId =" + permId + +" and roleId="+roleId, e); + } + } + + return permRoleRel; + } + + static public Permission getPermission(Permission permission) { Permission result = null; // @@ -138,10 +158,37 @@ public class AuthorizationStore { boolean result = false; try { - String csid = (String)JaxbUtils.getValue(entity, "getCsid"); - Object existingEntity = em.find(entity.getClass(), csid); - if (existingEntity != null) { - result = true; + if(entity instanceof Role) { + // If find by name, exists + Role roleEntity = (Role)entity; + String roleName = roleEntity.getRoleName(); + String tenantId = roleEntity.getTenantId(); + if(getRoleByName(em, roleName, tenantId)!=null) { + result = true; + logger.trace("Role {} already exists in tenant {}.", roleName, tenantId); + } else { + logger.trace("Role {} does not exist in tenant {}.", roleName, tenantId); + } + } else if(entity instanceof PermissionRoleRel) { + // If find by name, exists + PermissionRoleRel permRoleEntity = (PermissionRoleRel)entity; + String roleId = permRoleEntity.getRoleId(); + String permId = permRoleEntity.getPermissionId(); + if(getPermRoleRel(em, permId, roleId)!=null) { + result = true; + logger.trace("PermRoleRel for {}, {} already exists.", permId, roleId); + } else { + logger.trace("PermRoleRel for {}, {} does not exist.", permId, roleId); + } + } else { // Default case; also best test for Permission + String csid = (String)JaxbUtils.getValue(entity, "getCsid"); + Object existingEntity = em.find(entity.getClass(), csid); + if (existingEntity != null) { + result = true; + logger.trace("Entity with csid {} already exists.", csid); + } else { + logger.trace("Entity with csid {} does not exist.", csid); + } } } catch (Exception e) { //NOTE: Not all entities have a CSID attribute @@ -154,15 +201,20 @@ public class AuthorizationStore { */ public String store(EntityManager em, Object entity) throws Exception { boolean entityExists = exists(em, entity); + /* + * Logging moved to exists, for better detail if (entityExists == true) { - logger.debug("Entity to persist already exists."); + logger.trace("Entity to persist already exists."); } + */ if (JaxbUtils.getValue(entity, "getCreatedAt") == null) { JaxbUtils.setValue(entity, "setCreatedAtItem", Date.class, new Date()); } if (entityExists == true) { //em.merge(entity); FIXME: Leave commented out until we address CSPACE-5031 + // PLS: Question: why merge? what might be new to change, and is this really a good idea? + // Shouldn't we define them once and leave them alone? } else { em.persist(entity); } diff --git a/services/common/src/main/java/org/collectionspace/services/common/authorization_mgt/RoleStorageConstants.java b/services/common/src/main/java/org/collectionspace/services/common/authorization_mgt/RoleStorageConstants.java index c8812ee33..7a1b47157 100644 --- a/services/common/src/main/java/org/collectionspace/services/common/authorization_mgt/RoleStorageConstants.java +++ b/services/common/src/main/java/org/collectionspace/services/common/authorization_mgt/RoleStorageConstants.java @@ -34,4 +34,7 @@ public class RoleStorageConstants { final public static String ROLE_NAME = "roleName"; + final public static String PERM_ROLE_REL_ROLE_ID = "roleId"; + final public static String PERM_ROLE_REL_PERM_ID = "permissionId"; + } diff --git a/services/common/src/main/java/org/collectionspace/services/common/storage/jpa/JpaStorageUtils.java b/services/common/src/main/java/org/collectionspace/services/common/storage/jpa/JpaStorageUtils.java index b15f910c7..7a737a35b 100644 --- a/services/common/src/main/java/org/collectionspace/services/common/storage/jpa/JpaStorageUtils.java +++ b/services/common/src/main/java/org/collectionspace/services/common/storage/jpa/JpaStorageUtils.java @@ -324,6 +324,46 @@ public class JpaStorageUtils { return result; } + public static Object getEntityByDualKeys(EntityManager em, String entityName, + String key1, String value1, + String key2, String value2) { + return getEntityByDualKeys(em, entityName, key1, value1, key2, value2, null); + } + + public static Object getEntityByDualKeys(EntityManager em, String entityName, + String key1, String value1, + String key2, String value2, + String tenantId) { + Object result = null; + + if (entityName == null) { + throw new IllegalArgumentException("entityName is required"); + } + if (key1 == null || key2 == null) { + throw new IllegalArgumentException("key names are required"); + } + + StringBuilder queryStrBldr = new StringBuilder("SELECT a FROM "); + queryStrBldr.append(entityName); + queryStrBldr.append(" a"); + queryStrBldr.append(" WHERE " + key1 + " = :" + key1); + queryStrBldr.append(" AND " + key2 + " = :" + key2); + boolean csAdmin = SecurityUtils.isCSpaceAdmin(); + if (!csAdmin && tenantId != null) { + queryStrBldr.append(" AND tenantId = :tenantId"); + } + String queryStr = queryStrBldr.toString(); //for debugging + Query q = em.createQuery(queryStr); + q.setParameter(key1, value1); + q.setParameter(key2, value2); + if (!csAdmin) { + q.setParameter("tenantId", tenantId); + } + result = q.getSingleResult(); + + return result; + } + public static Object getEnityByKey(String entityName, String key, String value, String tenantId) { EntityManagerFactory emf = null;