From 0f676aa7f7d7ebfaf4b4716d16f7daa9216e21a6 Mon Sep 17 00:00:00 2001 From: Roy Wetherall Date: Mon, 8 Aug 2016 11:04:30 +1000 Subject: [PATCH] RM-3074: review comments --- .../security/ExtendedSecurityService.java | 2 +- .../security/ExtendedSecurityServiceImpl.java | 43 +++++++++---------- .../record/CreateInplaceRecordTest.java | 16 ++++--- 3 files changed, 31 insertions(+), 30 deletions(-) diff --git a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/security/ExtendedSecurityService.java b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/security/ExtendedSecurityService.java index 7f6e977f57..4960749eee 100644 --- a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/security/ExtendedSecurityService.java +++ b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/security/ExtendedSecurityService.java @@ -100,7 +100,7 @@ public interface ExtendedSecurityService * @param applyToParents true if extended security applied to parents (read only) false otherwise. * * @deprecated as of 2.5, because extended security is no longer applied to parents. Note that calling this method will - * only apply the exetended securiyt to the node and the applyToParents parameter value will be ignored. + * only apply the extended security to the node and the applyToParents parameter value will be ignored. * * @see #addExtendedSecurity(NodeRef, Set, Set) */ diff --git a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/security/ExtendedSecurityServiceImpl.java b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/security/ExtendedSecurityServiceImpl.java index 9b7aeb1311..b6d920021f 100644 --- a/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/security/ExtendedSecurityServiceImpl.java +++ b/rm-community/rm-community-repo/source/java/org/alfresco/module/org_alfresco_module_rm/security/ExtendedSecurityServiceImpl.java @@ -30,6 +30,7 @@ package org.alfresco.module.org_alfresco_module_rm.security; import static org.alfresco.service.cmr.security.PermissionService.GROUP_PREFIX; import java.util.Collections; +import java.util.HashSet; import java.util.List; import java.util.Set; @@ -52,13 +53,12 @@ import org.alfresco.service.cmr.security.AuthorityType; import org.alfresco.service.cmr.security.PermissionService; import org.alfresco.service.namespace.RegexQNamePattern; import org.alfresco.service.transaction.TransactionService; +import org.alfresco.util.Pair; import org.alfresco.util.ParameterCheck; import org.springframework.context.ApplicationListener; import org.springframework.context.event.ContextRefreshedEvent; import org.springframework.extensions.webscripts.ui.common.StringUtils; -import com.google.gdata.util.common.base.Pair; - /** * Extended security service implementation. * @@ -183,8 +183,8 @@ public class ExtendedSecurityServiceImpl extends ServiceBaseImpl Pair iprGroups = getIPRGroups(nodeRef); if (iprGroups != null) { - result = authorityService.getContainedAuthorities(null, iprGroups.first, true); - result.remove(iprGroups.second); + result = new HashSet(authorityService.getContainedAuthorities(null, iprGroups.getFirst(), true)); + result.remove(iprGroups.getSecond()); } else { @@ -206,7 +206,7 @@ public class ExtendedSecurityServiceImpl extends ServiceBaseImpl Pair iprGroups = getIPRGroups(nodeRef); if (iprGroups != null) { - result = authorityService.getContainedAuthorities(null, iprGroups.second, true); + result = authorityService.getContainedAuthorities(null, iprGroups.getSecond(), true); } else { @@ -235,10 +235,9 @@ public class ExtendedSecurityServiceImpl extends ServiceBaseImpl /** * Add extended security implementation method * - * @param nodeRef - * @param readers - * @param writers - * @param applyToParents + * @param nodeRef node reference + * @param readers readers set + * @param writers writers set */ private void addExtendedSecurityImpl(final NodeRef nodeRef, Set readers, Set writers) { @@ -249,8 +248,8 @@ public class ExtendedSecurityServiceImpl extends ServiceBaseImpl // assign groups to correct fileplan roles NodeRef filePlan = filePlanService.getFilePlan(nodeRef); - filePlanRoleService.assignRoleToAuthority(filePlan, FilePlanRoleService.ROLE_EXTENDED_READERS, iprGroups.first); - filePlanRoleService.assignRoleToAuthority(filePlan, FilePlanRoleService.ROLE_EXTENDED_WRITERS, iprGroups.second); + filePlanRoleService.assignRoleToAuthority(filePlan, FilePlanRoleService.ROLE_EXTENDED_READERS, iprGroups.getFirst()); + filePlanRoleService.assignRoleToAuthority(filePlan, FilePlanRoleService.ROLE_EXTENDED_WRITERS, iprGroups.getSecond()); // assign groups to node assignIPRGroupsToNode(iprGroups, nodeRef); @@ -273,7 +272,7 @@ public class ExtendedSecurityServiceImpl extends ServiceBaseImpl * Return null if none found. * * @param nodeRef node reference - * @return Pair where first if the read group and second if the write group, null if none found + * @return Pair where first is the read group and second if the write group, null if none found */ private Pair getIPRGroups(NodeRef nodeRef) { @@ -322,18 +321,18 @@ public class ExtendedSecurityServiceImpl extends ServiceBaseImpl Pair result = null; // find read group or determine what the next index is if no group exists or there is a clash - Pair readGroupResult = findIPRGoup(READER_GROUP_PREFIX, readers, writers); + Pair readGroupResult = findIPRGroup(READER_GROUP_PREFIX, readers, writers); - if (readGroupResult.first == null) + if (readGroupResult.getFirst() == null) { // create inplace record reader and writer groups - result = createIPRGroups(readers, writers, readGroupResult.second); + result = createIPRGroups(readers, writers, readGroupResult.getSecond()); } else { // set result - result = new Pair(readGroupResult.first, - getIRPWriteGroupNameFromReadGroupName(readGroupResult.first, readers, writers)); + result = new Pair(readGroupResult.getFirst(), + getIRPWriteGroupNameFromReadGroupName(readGroupResult.getFirst(), readers, writers)); } return result; @@ -352,7 +351,7 @@ public class ExtendedSecurityServiceImpl extends ServiceBaseImpl * @return Pair where first is the name of the found group, null if none found and second * if the next available create index */ - private Pair findIPRGoup(String groupPrefix, Set readers, Set writers) + private Pair findIPRGroup(String groupPrefix, Set readers, Set writers) { String iprGroup = null; int nextGroupIndex = 0; @@ -575,8 +574,8 @@ public class ExtendedSecurityServiceImpl extends ServiceBaseImpl */ private void assignIPRGroupsToNode(Pair iprGroups, NodeRef nodeRef) { - permissionService.setPermission(nodeRef, iprGroups.first, RMPermissionModel.READ_RECORDS, true); - permissionService.setPermission(nodeRef, iprGroups.second, RMPermissionModel.FILING, true); + permissionService.setPermission(nodeRef, iprGroups.getFirst(), RMPermissionModel.READ_RECORDS, true); + permissionService.setPermission(nodeRef, iprGroups.getSecond(), RMPermissionModel.FILING, true); } /** @@ -615,8 +614,8 @@ public class ExtendedSecurityServiceImpl extends ServiceBaseImpl if (iprGroups != null) { // remove group permissions from node - permissionService.clearPermission(nodeRef, iprGroups.first); - permissionService.clearPermission(nodeRef, iprGroups.second); + permissionService.clearPermission(nodeRef, iprGroups.getFirst()); + permissionService.clearPermission(nodeRef, iprGroups.getSecond()); // TODO delete the groups if they are no longer in use (easier said than done perhaps!) } diff --git a/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/integration/record/CreateInplaceRecordTest.java b/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/integration/record/CreateInplaceRecordTest.java index ca818eedc2..0394e67af0 100644 --- a/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/integration/record/CreateInplaceRecordTest.java +++ b/rm-community/rm-community-repo/test/java/org/alfresco/module/org_alfresco_module_rm/test/integration/record/CreateInplaceRecordTest.java @@ -61,7 +61,7 @@ public class CreateInplaceRecordTest extends BaseRMTestCase public void given() { // Check that the document is not a record - assertFalse(recordService.isRecord(dmDocument)); + assertFalse("The document should not be a record", recordService.isRecord(dmDocument)); } public void when() @@ -82,7 +82,7 @@ public class CreateInplaceRecordTest extends BaseRMTestCase public void then() { // Check that the document is a record now - assertTrue(recordService.isRecord(dmDocument)); + assertTrue("The document should now be a record", recordService.isRecord(dmDocument)); // Check that the record is in the unfiled container @@ -122,7 +122,7 @@ public class CreateInplaceRecordTest extends BaseRMTestCase public void given() { // Check that the document is not a record - assertFalse(recordService.isRecord(dmDocument)); + assertFalse("The document should not be a record", recordService.isRecord(dmDocument)); // Declare the document as a record AuthenticationUtil.runAs(new RunAsWork() @@ -137,8 +137,8 @@ public class CreateInplaceRecordTest extends BaseRMTestCase }, dmCollaborator); // Check that the document is a record - assertTrue(recordService.isRecord(dmDocument)); - assertFalse(recordService.isFiled(dmDocument)); + assertTrue("The document should be a record", recordService.isRecord(dmDocument)); + assertFalse("The record should not be filed", recordService.isFiled(dmDocument)); } public void when() throws FileExistsException, FileNotFoundException @@ -150,12 +150,14 @@ public class CreateInplaceRecordTest extends BaseRMTestCase public void then() { // Check that the document is a record now - assertTrue(recordService.isRecord(dmDocument)); - assertTrue(recordService.isFiled(dmDocument)); + assertTrue("The document should be a record", recordService.isRecord(dmDocument)); + assertTrue("The record hsould be filed", recordService.isFiled(dmDocument)); // Check that the record is in the unfiled container + // TODO // Check that the record is still a child of the collaboration folder + // TODO // Check that the collaborator has filling permissions on the record AuthenticationUtil.runAs(new RunAsWork()