From 2220fd889e15c16d8855fa440697f34e78e4f035 Mon Sep 17 00:00:00 2001 From: Tuna Aksoy Date: Tue, 27 Jun 2017 16:35:51 +0100 Subject: [PATCH 1/3] RM-5346: Null pointers should not be dereferenced --- .../capability/RMAfterInvocationProvider.java | 10 ++++++++-- .../script/ApplyDodCertModelFixesGet.java | 9 +++++++++ .../script/ApplyFixMob1573Get.java | 4 ++++ 3 files changed, 21 insertions(+), 2 deletions(-) diff --git a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/capability/RMAfterInvocationProvider.java b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/capability/RMAfterInvocationProvider.java index 784ac9b778..92acabb7c3 100644 --- a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/capability/RMAfterInvocationProvider.java +++ b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/capability/RMAfterInvocationProvider.java @@ -37,6 +37,7 @@ import java.util.Map; import java.util.Set; import java.util.StringTokenizer; +import org.alfresco.error.AlfrescoRuntimeException; import org.alfresco.module.org_alfresco_module_rm.model.RecordsManagementModel; import org.alfresco.repo.search.SimpleResultSetMetaData; import org.alfresco.repo.search.impl.lucene.PagingLuceneResultSet; @@ -172,7 +173,12 @@ public class RMAfterInvocationProvider extends RMSecurityCommon } else if (StoreRef.class.isAssignableFrom(returnedObject.getClass())) { - return decide(authentication, object, config, nodeService.getRootNode((StoreRef) returnedObject)).getStoreRef(); + NodeRef rootNodeRef = decide(authentication, object, config, nodeService.getRootNode((StoreRef) returnedObject)); + if (rootNodeRef == null) + { + throw new AlfrescoRuntimeException("Root node reference of '" + returnedObject + "' is null."); + } + return rootNodeRef.getStoreRef(); } else if (NodeRef.class.isAssignableFrom(returnedObject.getClass())) { @@ -208,7 +214,7 @@ public class RMAfterInvocationProvider extends RMSecurityCommon } else { - if (logger.isDebugEnabled()) + if (logger.isDebugEnabled() && object.getClass() != null) { logger.debug("Uncontrolled object - access allowed for " + object.getClass().getName()); } diff --git a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/script/ApplyDodCertModelFixesGet.java b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/script/ApplyDodCertModelFixesGet.java index b22f92feb1..4814e083e3 100644 --- a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/script/ApplyDodCertModelFixesGet.java +++ b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/script/ApplyDodCertModelFixesGet.java @@ -105,6 +105,15 @@ public class ApplyDodCertModelFixesGet extends DeclarativeWebScript } M2Model customModel = readCustomContentModel(); + if (customModel == null) + { + final String msg = "Custom content model could not be read"; + if (logger.isErrorEnabled()) + { + logger.error(msg); + } + throw new AlfrescoRuntimeException(msg); + } String customAspectName = ASPECT_CUSTOM_ASSOCIATIONS.toPrefixString(namespaceService); M2Aspect customAssocsAspect = customModel.getAspect(customAspectName); diff --git a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/script/ApplyFixMob1573Get.java b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/script/ApplyFixMob1573Get.java index d8dfabbd35..e4e28d66a7 100644 --- a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/script/ApplyFixMob1573Get.java +++ b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/script/ApplyFixMob1573Get.java @@ -83,6 +83,10 @@ public class ApplyFixMob1573Get extends DeclarativeWebScript public Map executeImpl(WebScriptRequest req, Status status, Cache cache) { M2Model customModel = readCustomContentModel(); + if (customModel == null) + { + throw new AlfrescoRuntimeException("Custom content model could not be read"); + } // Go through every custom reference defined in the custom model and make sure that it // has many-to-many multiplicity From 965539d8f238831dbf7faa27efd718c4b235e828 Mon Sep 17 00:00:00 2001 From: Tuna Aksoy Date: Tue, 27 Jun 2017 16:52:51 +0100 Subject: [PATCH 2/3] RM-5347: Resources should be closed --- .../audit/RecordsManagementAuditServiceImpl.java | 9 ++++++++- .../org_alfresco_module_rm/script/TransferReportGet.java | 8 +++++++- .../script/TransferReportPost.java | 8 +++++++- 3 files changed, 22 insertions(+), 3 deletions(-) diff --git a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/audit/RecordsManagementAuditServiceImpl.java b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/audit/RecordsManagementAuditServiceImpl.java index d7041ea2d9..56edc5a6ac 100644 --- a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/audit/RecordsManagementAuditServiceImpl.java +++ b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/audit/RecordsManagementAuditServiceImpl.java @@ -774,11 +774,13 @@ public class RecordsManagementAuditServiceImpl extends AbstractLifecycleBean ParameterCheck.mandatory("params", params); Writer fileWriter = null; + FileOutputStream fileOutputStream = null; try { File auditTrailFile = TempFileProvider.createTempFile(AUDIT_TRAIL_FILE_PREFIX, format == ReportFormat.HTML ? AUDIT_TRAIL_HTML_FILE_SUFFIX : AUDIT_TRAIL_JSON_FILE_SUFFIX); - fileWriter = new BufferedWriter(new OutputStreamWriter(new FileOutputStream(auditTrailFile),"UTF8")); + fileOutputStream = new FileOutputStream(auditTrailFile); + fileWriter = new BufferedWriter(new OutputStreamWriter(fileOutputStream,"UTF8")); // Get the results, dumping to file getAuditTrailImpl(params, null, fileWriter, format); // Done @@ -790,6 +792,11 @@ public class RecordsManagementAuditServiceImpl extends AbstractLifecycleBean } finally { + // close the file output stream + if (fileOutputStream != null) + { + try { fileOutputStream.close(); } catch (IOException closeEx) {} + } // close the writer if (fileWriter != null) { diff --git a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/script/TransferReportGet.java b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/script/TransferReportGet.java index f4f456035f..41c0b2bdb4 100644 --- a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/script/TransferReportGet.java +++ b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/script/TransferReportGet.java @@ -127,6 +127,7 @@ public class TransferReportGet extends BaseTransferWebScript { File report = TempFileProvider.createTempFile(REPORT_FILE_PREFIX, REPORT_FILE_SUFFIX); Writer writer = null; + FileOutputStream fileOutputStream = null; try { // get all 'transferred' nodes @@ -139,7 +140,8 @@ public class TransferReportGet extends BaseTransferWebScript } // create the writer - writer = new OutputStreamWriter(new FileOutputStream(report), Charset.forName("UTF-8")); + fileOutputStream = new FileOutputStream(report); + writer = new OutputStreamWriter(fileOutputStream, Charset.forName("UTF-8")); // use RMService to get disposition authority String dispositionAuthority = null; @@ -172,6 +174,10 @@ public class TransferReportGet extends BaseTransferWebScript } finally { + if (fileOutputStream != null) + { + try { fileOutputStream.close(); } catch (IOException ioe) {} + } if (writer != null) { try { writer.close(); } catch (IOException ioe) {} diff --git a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/script/TransferReportPost.java b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/script/TransferReportPost.java index 673ffb8241..de5b6db017 100644 --- a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/script/TransferReportPost.java +++ b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/script/TransferReportPost.java @@ -235,6 +235,7 @@ public class TransferReportPost extends BaseTransferWebScript { File report = TempFileProvider.createTempFile(REPORT_FILE_PREFIX, REPORT_FILE_SUFFIX); Writer writer = null; + FileOutputStream fileOutputStream = null; try { // get all 'transferred' nodes @@ -247,7 +248,8 @@ public class TransferReportPost extends BaseTransferWebScript } // create the writer - writer = new OutputStreamWriter(new FileOutputStream(report), Charset.forName("UTF-8")); + fileOutputStream = new FileOutputStream(report); + writer = new OutputStreamWriter(fileOutputStream, Charset.forName("UTF-8")); // use RMService to get disposition authority String dispositionAuthority = null; @@ -324,6 +326,10 @@ public class TransferReportPost extends BaseTransferWebScript } finally { + if (fileOutputStream != null) + { + try { fileOutputStream.close(); } catch (IOException ioe) {} + } if (writer != null) { try { writer.close(); } catch (IOException ioe) {} From 16ead6e811ab9f439adc0bfa90f23c72ac3faffc Mon Sep 17 00:00:00 2001 From: Tuna Aksoy Date: Wed, 28 Jun 2017 16:03:38 +0100 Subject: [PATCH 3/3] RM-5345: Fixed review comments --- .../RecordsManagementAuditServiceImpl.java | 25 +++---------- .../capability/RMAfterInvocationProvider.java | 2 +- .../script/ApplyDodCertModelFixesGet.java | 37 +++++-------------- .../script/TransferReportGet.java | 22 ++--------- .../script/TransferReportPost.java | 22 ++--------- 5 files changed, 23 insertions(+), 85 deletions(-) diff --git a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/audit/RecordsManagementAuditServiceImpl.java b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/audit/RecordsManagementAuditServiceImpl.java index 56edc5a6ac..ab4643f1bd 100644 --- a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/audit/RecordsManagementAuditServiceImpl.java +++ b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/audit/RecordsManagementAuditServiceImpl.java @@ -773,14 +773,12 @@ public class RecordsManagementAuditServiceImpl extends AbstractLifecycleBean { ParameterCheck.mandatory("params", params); - Writer fileWriter = null; - FileOutputStream fileOutputStream = null; - try + File auditTrailFile = TempFileProvider.createTempFile(AUDIT_TRAIL_FILE_PREFIX, + format == ReportFormat.HTML ? AUDIT_TRAIL_HTML_FILE_SUFFIX : AUDIT_TRAIL_JSON_FILE_SUFFIX); + + try (FileOutputStream fileOutputStream = new FileOutputStream(auditTrailFile); + Writer fileWriter = new BufferedWriter(new OutputStreamWriter(fileOutputStream,"UTF8"));) { - File auditTrailFile = TempFileProvider.createTempFile(AUDIT_TRAIL_FILE_PREFIX, - format == ReportFormat.HTML ? AUDIT_TRAIL_HTML_FILE_SUFFIX : AUDIT_TRAIL_JSON_FILE_SUFFIX); - fileOutputStream = new FileOutputStream(auditTrailFile); - fileWriter = new BufferedWriter(new OutputStreamWriter(fileOutputStream,"UTF8")); // Get the results, dumping to file getAuditTrailImpl(params, null, fileWriter, format); // Done @@ -790,19 +788,6 @@ public class RecordsManagementAuditServiceImpl extends AbstractLifecycleBean { throw new AlfrescoRuntimeException(MSG_TRAIL_FILE_FAIL, e); } - finally - { - // close the file output stream - if (fileOutputStream != null) - { - try { fileOutputStream.close(); } catch (IOException closeEx) {} - } - // close the writer - if (fileWriter != null) - { - try { fileWriter.close(); } catch (IOException closeEx) {} - } - } } /** diff --git a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/capability/RMAfterInvocationProvider.java b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/capability/RMAfterInvocationProvider.java index 92acabb7c3..b9de6dfeca 100644 --- a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/capability/RMAfterInvocationProvider.java +++ b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/capability/RMAfterInvocationProvider.java @@ -214,7 +214,7 @@ public class RMAfterInvocationProvider extends RMSecurityCommon } else { - if (logger.isDebugEnabled() && object.getClass() != null) + if (logger.isDebugEnabled() && object != null) { logger.debug("Uncontrolled object - access allowed for " + object.getClass().getName()); } diff --git a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/script/ApplyDodCertModelFixesGet.java b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/script/ApplyDodCertModelFixesGet.java index 4814e083e3..fba2b94757 100644 --- a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/script/ApplyDodCertModelFixesGet.java +++ b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/script/ApplyDodCertModelFixesGet.java @@ -51,8 +51,8 @@ import org.alfresco.service.cmr.repository.ContentService; import org.alfresco.service.cmr.repository.ContentWriter; import org.alfresco.service.cmr.repository.NodeRef; import org.alfresco.service.namespace.NamespaceService; -import org.apache.commons.logging.Log; -import org.apache.commons.logging.LogFactory; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.springframework.extensions.webscripts.Cache; import org.springframework.extensions.webscripts.DeclarativeWebScript; import org.springframework.extensions.webscripts.Status; @@ -81,7 +81,7 @@ public class ApplyDodCertModelFixesGet extends DeclarativeWebScript private static final String RMC_CUSTOM_RECORD_PROPERTIES = RecordsManagementCustomModel.RM_CUSTOM_PREFIX + ":customRecordProperties"; /** Logger */ - private static Log logger = LogFactory.getLog(ApplyDodCertModelFixesGet.class); + private static final Logger LOGGER = LoggerFactory.getLogger(ApplyDodCertModelFixesGet.class); private ContentService contentService; private NamespaceService namespaceService; @@ -99,19 +99,13 @@ public class ApplyDodCertModelFixesGet extends DeclarativeWebScript @Override public Map executeImpl(WebScriptRequest req, Status status, Cache cache) { - if (logger.isInfoEnabled()) - { - logger.info("Applying webscript-based patches to RM custom model in the repo."); - } + LOGGER.info("Applying webscript-based patches to RM custom model in the repo."); M2Model customModel = readCustomContentModel(); if (customModel == null) { final String msg = "Custom content model could not be read"; - if (logger.isErrorEnabled()) - { - logger.error(msg); - } + LOGGER.error(msg); throw new AlfrescoRuntimeException(msg); } @@ -121,19 +115,12 @@ public class ApplyDodCertModelFixesGet extends DeclarativeWebScript if (customAssocsAspect == null) { final String msg = "Unknown aspect: " + customAspectName; - if (logger.isErrorEnabled()) - { - logger.error(msg); - } + LOGGER.error(msg); throw new AlfrescoRuntimeException(msg); } - // MOB-1573. All custom references should have many-many multiplicity. - if (logger.isInfoEnabled()) - { - logger.info("MOB-1573. All custom references should have many-many multiplicity."); - } + LOGGER.info("MOB-1573. All custom references should have many-many multiplicity."); for (M2ClassAssociation classAssoc : customAssocsAspect.getAssociations()) { @@ -143,10 +130,7 @@ public class ApplyDodCertModelFixesGet extends DeclarativeWebScript } //MOB-1621. Custom fields should be created as untokenized by default. - if (logger.isInfoEnabled()) - { - logger.info("MOB-1621. Custom fields should be created as untokenized by default."); - } + LOGGER.info("MOB-1621. Custom fields should be created as untokenized by default."); List allCustomPropertiesAspects = new ArrayList(4); allCustomPropertiesAspects.add(RMC_CUSTOM_RECORD_SERIES_PROPERTIES); @@ -169,10 +153,7 @@ public class ApplyDodCertModelFixesGet extends DeclarativeWebScript writeCustomContentModel(customModel); - if (logger.isInfoEnabled()) - { - logger.info("Completed application of webscript-based patches to RM custom model in the repo."); - } + LOGGER.info("Completed application of webscript-based patches to RM custom model in the repo."); Map model = new HashMap(1, 1.0f); model.put("success", true); diff --git a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/script/TransferReportGet.java b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/script/TransferReportGet.java index 41c0b2bdb4..352ef597d3 100644 --- a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/script/TransferReportGet.java +++ b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/script/TransferReportGet.java @@ -126,9 +126,10 @@ public class TransferReportGet extends BaseTransferWebScript File generateJSONTransferReport(NodeRef transferNode) throws IOException { File report = TempFileProvider.createTempFile(REPORT_FILE_PREFIX, REPORT_FILE_SUFFIX); - Writer writer = null; - FileOutputStream fileOutputStream = null; - try + + // create the writer + try (FileOutputStream fileOutputStream = new FileOutputStream(report); + Writer writer = new OutputStreamWriter(fileOutputStream, Charset.forName("UTF-8"));) { // get all 'transferred' nodes NodeRef[] itemsToTransfer = getTransferNodes(transferNode); @@ -139,10 +140,6 @@ public class TransferReportGet extends BaseTransferWebScript " items into file: " + report.getAbsolutePath()); } - // create the writer - fileOutputStream = new FileOutputStream(report); - writer = new OutputStreamWriter(fileOutputStream, Charset.forName("UTF-8")); - // use RMService to get disposition authority String dispositionAuthority = null; if (itemsToTransfer.length > 0) @@ -172,17 +169,6 @@ public class TransferReportGet extends BaseTransferWebScript // write the JSON footer writer.write("\n\t\t]\n\t}\n}"); } - finally - { - if (fileOutputStream != null) - { - try { fileOutputStream.close(); } catch (IOException ioe) {} - } - if (writer != null) - { - try { writer.close(); } catch (IOException ioe) {} - } - } return report; } diff --git a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/script/TransferReportPost.java b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/script/TransferReportPost.java index de5b6db017..2d762e0d4c 100644 --- a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/script/TransferReportPost.java +++ b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/script/TransferReportPost.java @@ -234,9 +234,10 @@ public class TransferReportPost extends BaseTransferWebScript File generateHTMLTransferReport(NodeRef transferNode) throws IOException { File report = TempFileProvider.createTempFile(REPORT_FILE_PREFIX, REPORT_FILE_SUFFIX); - Writer writer = null; - FileOutputStream fileOutputStream = null; - try + + // create the writer + try (FileOutputStream fileOutputStream = new FileOutputStream(report) ; + Writer writer = new OutputStreamWriter(fileOutputStream, Charset.forName("UTF-8"));) { // get all 'transferred' nodes NodeRef[] itemsToTransfer = getTransferNodes(transferNode); @@ -247,10 +248,6 @@ public class TransferReportPost extends BaseTransferWebScript " items into file: " + report.getAbsolutePath()); } - // create the writer - fileOutputStream = new FileOutputStream(report); - writer = new OutputStreamWriter(fileOutputStream, Charset.forName("UTF-8")); - // use RMService to get disposition authority String dispositionAuthority = null; if (itemsToTransfer.length > 0) @@ -324,17 +321,6 @@ public class TransferReportPost extends BaseTransferWebScript // write the HTML footer writer.write(""); } - finally - { - if (fileOutputStream != null) - { - try { fileOutputStream.close(); } catch (IOException ioe) {} - } - if (writer != null) - { - try { writer.close(); } catch (IOException ioe) {} - } - } return report; }